Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

System.Net.Requests: Pick correct RemoteExecutor overload in test #74994

Merged
merged 1 commit into from
Sep 4, 2022

Conversation

akoeplinger
Copy link
Member

@akoeplinger akoeplinger commented Sep 2, 2022

The test was using RemoteExecutor.Invoke(Action<string, string, string, string> method, ...) since there is no overload that takes Func<string, string, string, string, Task> (only one with three strings) and that means it's becoming an async void.

The delegate that gets invoked will return the moment the method awaits something not yet completed, so now there's a race condition, where RemoteExecutor.Invoke thinks all work is done, but there's still likely work running and it'll start doing all its cleanup stuff like killing child processes.

Fix by removing one string parameter so it picks the correct overload. I'll also open an arcade PR to add an overload with four string arguments.

Fixes #74667

The test was using `RemoteExecutor.Invoke(Action<string, string, string, string> method, ...)` since there is no overload that takes `Func<string, string, string, string, Task>` (only one with three strings) so we weren't awaiting the Task.

Fix by removing one string parameter so it picks the correct overload.

Fixes dotnet#74667
@ghost
Copy link

ghost commented Sep 2, 2022

Tagging subscribers to this area: @dotnet/ncl
See info in area-owners.md if you want to be subscribed.

Issue Details

The test was using RemoteExecutor.Invoke(Action<string, string, string, string> method, ...) since there is no overload that takes Func<string, string, string, string, Task> (only one with three strings) so we weren't awaiting the Task.

Fix by removing one string parameter so it picks the correct overload.

Fixes #74667

Author: akoeplinger
Assignees: -
Labels:

area-System.Net

Milestone: -

@akoeplinger akoeplinger marked this pull request as ready for review September 2, 2022 13:27
@tmds
Copy link
Member

tmds commented Sep 2, 2022

The delegate that gets invoked will return the moment the method awaits something not yet completed, so now there's a race condition, where RemoteExecutor.Invoke thinks all work is done, but there's still likely work running and it'll start doing all its cleanup stuff like killing child processes.

For the parent, that means the child is short-lived. It shouldn't cause a situation where the child goes missing (Error while reaping child. errno = 10).

This could be triggering the issue, but it is not the root cause.

@akoeplinger akoeplinger merged commit 46875f5 into dotnet:main Sep 4, 2022
@akoeplinger akoeplinger deleted the netrequests branch September 4, 2022 17:25
akoeplinger added a commit that referenced this pull request Sep 5, 2022
They were disabled but it looks like the underlying problem was fixed since they pass now.
Noticed while investigating #74994

Closes #34690
@akoeplinger
Copy link
Member Author

@tmds you were right, we're still seeing this on Mono in CI even after this fix so this is actually an unrelated issue. I'll disable the tests again.

akoeplinger added a commit to dotnet/arcade that referenced this pull request Sep 6, 2022
In dotnet/runtime#74994 we hit an issue because a test was inadvertently using `RemoteExecutor.Invoke(Action<string, string, string, string> method, ...)` since there was no overload that takes `Func<string, string, string, string, Task>` (only one with three strings) and that means it's becoming an async void (bad!).

This PR makes sure we have a matching number of overloads that take Func with the same number of arguments as the one that takes Action.

Since there was a bit of a mismatch (one overload took five arguments), I made the overloads consistent so that everyone takes five args.
@ghost ghost locked as resolved and limited conversation to collaborators Oct 5, 2022
@karelz karelz added this to the 8.0.0 milestone Mar 22, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Mono crash in System.Net.Requests tests on linux-arm64
4 participants