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

[SslStream] Dispose seems to call blocking IO #61506

Closed
ManickaP opened this issue Nov 12, 2021 · 8 comments · Fixed by #61530
Closed

[SslStream] Dispose seems to call blocking IO #61506

ManickaP opened this issue Nov 12, 2021 · 8 comments · Fixed by #61530

Comments

@ManickaP
Copy link
Member

ManickaP commented Nov 12, 2021

In particular that it looks like blocking I/O is happening as part of that cleanup. That’s also something we should strive to avoid.

dispose

We should investigate and see if we can get rid of the blocking IO.

Discovered during investigation of the same problem as #61505

cc: @stephentoub @wfurt

@dotnet-issue-labeler
Copy link

I couldn't figure out the best area label to add to this issue. If you have write-permissions please help me learn by adding exactly one area label.

@dotnet-issue-labeler dotnet-issue-labeler bot added the untriaged New issue has not been triaged by the area owner label Nov 12, 2021
@ghost
Copy link

ghost commented Nov 12, 2021

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

Issue Details

In particular that it looks like blocking I/O is happening as part of that cleanup. That’s also something we should strive to avoid.

dispose

We should investigate and see if we can get rid of the blocking IO.

Discovered during investigation of the same problem as #61505

cc: @stephentoub @wfurt

Author: ManickaP
Assignees: -
Labels:

bug, area-System.Net.Security, untriaged

Milestone: -

@karelz karelz added this to the 7.0.0 milestone Nov 12, 2021
@ManickaP ManickaP removed the untriaged New issue has not been triaged by the area owner label Nov 12, 2021
@geoffkizer
Copy link
Contributor

We should investigate and see if we can get rid of the blocking IO.

I don't think we can get rid of the blocking IO here since it's just an LRPC call deep in the SCHANNEL code.

I think what we care about here is ensuring that these calls (i.e. calling Dispose on the connection's stream, which is calling SslStream.Close in this case) are not blocking any important processing or cleanup that should be occurring.

The code is already structured to gather up all the connections to dispose and then dispose them outside of the connection pool lock, so we are not holding any locks here that would prevent someone from using the connection pool, which is good.

However, we are blocking RemoveStalePools() from being able to continue scavenging other connection pools until these connections get disposed.

I would suggest we just spawn a background Task to actually dispose the connections and then return back to RemoveStalePools without waiting for it to complete. This should allow RemoveStalePools to make progress more quickly, and hopefully result in better scavenging of pools and connections overall.

@geoffkizer
Copy link
Contributor

BTW, this seems like a good thing to do regardless of blocking or not. Even for non-SSL cases, NetworkStream.Dispose will require a kernel call and that's never cheap.

@ManickaP ManickaP self-assigned this Nov 22, 2021
@ghost ghost added the in-pr There is an active PR which will close this issue when it is merged label Nov 30, 2021
@karelz karelz modified the milestones: 7.0.0, 6.0.x Nov 30, 2021
@karelz
Copy link
Member

karelz commented Nov 30, 2021

It was fixed in main (7.0.0) in PR #61530

@stephentoub
Copy link
Member

stephentoub commented Nov 30, 2021

I don't think we can get rid of the blocking IO here since it's just an LRPC call deep in the SCHANNEL code.

Can we validate there's no other option, no way to achieve this asynchronously? Some other API we should be calling, or some other option we can set, or some such thing? It's good that it's now not blocking cleaning up other connections in the cleanup loop, but it's still blocking a thread pool thread.

@wfurt
Copy link
Member

wfurt commented Nov 30, 2021

I don't think we can get rid of the blocking IO here since it's just an LRPC call deep in the SCHANNEL code.

Can we validate there's no other option, no way to achieve this asynchronously? Some other API we should be calling, or some other option we can set, or some such thing? It's good that it's now not blocking cleaning up other connections in the cleanup loop, but it's still blocking a thread pool thread.

I can take a look but AFAIK all the APIs are synchronous. The fact that it happens in different process are completely opaque to caller.

@ghost ghost removed the in-pr There is an active PR which will close this issue when it is merged label Dec 15, 2021
@karelz
Copy link
Member

karelz commented Dec 16, 2021

Fixed in main (7.0) in PR #61530 and in 6.0.2 in PR #62008.

@karelz karelz closed this as completed Dec 16, 2021
@ghost ghost locked as resolved and limited conversation to collaborators Jan 15, 2022
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