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

Cancellation of Socket.ConnectAsync intermitently hangs #42198

Closed
ManickaP opened this issue Sep 14, 2020 · 4 comments · Fixed by #42919
Closed

Cancellation of Socket.ConnectAsync intermitently hangs #42198

ManickaP opened this issue Sep 14, 2020 · 4 comments · Fixed by #42919

Comments

@ManickaP
Copy link
Member

When Socket.ConnectAsync is cancelled it might lead to a deadlock.

The cancellation code for MultipleConnectAsync tries to acquire a lock which is held by the connectAsync continuation (triggered by DoDnsCallback). The continuation then tries to dispose of the cancellation registration which will wait for all the callbacks if they've already been triggered.

This is easily reproducible with our HTTP stress suite (for HTTP 1.1 runs longer than 30 mins).

The biggest impact of this issue is that it prevents us from getting reasonable results from HTTP 1.1 stress pipeline on master. The pipeline gets cancelled on timeout and no results are available (AzDO empty logs).

@ManickaP ManickaP added this to the 6.0.0 milestone Sep 14, 2020
@Dotnet-GitSync-Bot Dotnet-GitSync-Bot added the untriaged New issue has not been triaged by the area owner label Sep 14, 2020
@ManickaP ManickaP removed the untriaged New issue has not been triaged by the area owner label Sep 14, 2020
@ghost
Copy link

ghost commented Sep 14, 2020

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

@geoffkizer geoffkizer changed the title Socket.CancelAsync intermitently hangs Socket.CancelConnectAsync intermitently hangs Sep 14, 2020
@geoffkizer
Copy link
Contributor

In AttemptConnection, when ConnectAsync completes synchronously, we are calling InternalConnectCallback directly. This is bad since the caller of AttemptConnection should be holding the lock here.

We should instead return to the caller and do the call to InternalConnectCallback outside the lock.

Additionally, we should add asserts wherever we take the lock, but especially in the callbacks like InternalConnectCallback, that the lock is not already held by this thread -- basically, Debug.Assert(!Monitor.IsEntered(_lockObject)).

And if a method is assuming the caller is holding the lock, like AttemptConnection is, we should add an assert that the lock is actually held. This is valuable not just to detect bugs, but also to document the expectation of the function.

@karelz karelz added the bug label Sep 15, 2020
@ManickaP ManickaP changed the title Socket.CancelConnectAsync intermitently hangs Cancelltion of Socket.ConnectAsync intermitently hangs Sep 16, 2020
@ManickaP ManickaP changed the title Cancelltion of Socket.ConnectAsync intermitently hangs Cancellation of Socket.ConnectAsync intermitently hangs Sep 16, 2020
ManickaP added a commit that referenced this issue Sep 23, 2020
Fixes:
    stress client double read of content fixed
    fixed stress client hangs at start and stop
    leveraged HttpVersionPolicy
    increased pipeline timeout since we doubled the runs
    fixed base docker images to avoid missing IO.Pipelines Kestrel exception.

Re-hauled tracing:
    added server file logging
    added log file rotation

Minor renames.

Contributes to: #42211 and #42198
@riberk
Copy link

riberk commented Oct 19, 2020

Is there a workaround? I have the same problem and i don`t know how to solve it from outside of the Socket class

@ManickaP
Copy link
Member Author

ManickaP commented Nov 6, 2020

The only workaround is to avoid cancellation during socket connect. I don't know your exact problem so I cannot help much further at this point.

@ghost ghost locked as resolved and limited conversation to collaborators Dec 7, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants