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

[HTTP] Scavange fix #61530

Merged
merged 3 commits into from
Nov 22, 2021
Merged

Conversation

ManickaP
Copy link
Member

@ManickaP ManickaP commented Nov 12, 2021

Disposes connection in a separate task to not to block the scavenging timer callback on the stream close.
Guards scavenging timer callback from parallel execution.

Fixes #61505
Fixes #61506

I plan to validate the fix with the customer before merging. But I'd like to know if the way I solved this is not completely wrong or too different from what you had in mind? @geoffkizer @stephentoub

@ghost
Copy link

ghost commented Nov 12, 2021

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

Issue Details

Disposes connection in a separate task to not to block the scavenging timer callback on the stream close.
Guards scavenging timer callback from parallel execution.

Fixes #61505 and #61506

I plan to validate the fix with the customer before merging. But I'd like to know if the way I solved this is not completely wrong or too different from what you had in mind? @geoffkizer @stephentoub

Author: ManickaP
Assignees: -
Labels:

area-System.Net.Http

Milestone: -

@geoffkizer
Copy link
Contributor

Rather than using a periodic timer and then checking for parallel execution, I would suggest we just use a single-shot timer and reset it in RemoveStalePools after the scavenge pass has completed.

// Dispose them asynchronously to not to block the caller on closing the SslStream or NetworkStream.
if (toDispose is not null)
{
Task.Run(() => toDispose.ForEach(c => c.Dispose()));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This unnecessarily allocates a closure/delegate even if toDispose is null. That can be avoided with:

Suggested change
Task.Run(() => toDispose.ForEach(c => c.Dispose()));
Task.Factory.StartNew(s => ((List<HttpConnectionBase>)s!).ForEach(c => c.Dispose()), toDispose,
CancellationToken.None, TaskCreationOptions.DenyChildAttach, TaskScheduler.Default);

@ManickaP ManickaP force-pushed the mapichov/61505_no_parallel_scavenge branch 2 times, most recently from c9de343 to f3cb3ae Compare November 15, 2021 18:16
// such that any connections returned to the pool to be cached will be disposed of. In such
// a case, we also remove the pool from the set of pools to avoid a leak.
foreach (KeyValuePair<HttpConnectionKey, HttpConnectionPool> entry in _pools)
try
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why was this added?

If it's possible for this code to throw, then it seems like we have a deeper problem here, which is that we could skip cleaning up some pools.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll remove it.
I didn't want to accidentally end up with turned off cleaning timer which never gets restarted.

@geoffkizer
Copy link
Contributor

One issue above, otherwise LGTM.

@ManickaP ManickaP force-pushed the mapichov/61505_no_parallel_scavenge branch from f3cb3ae to 5013728 Compare November 15, 2021 19:40
@ManickaP
Copy link
Member Author

Customer validated the fix against 6.0 and 5.0. It helped with overlapping callbacks. However, in case of 5.0, they're still seeing abnormal number of HttpConnectionPool instances even after scavenging (this doesn't happen in 6.0 neither before nor after this change).

@ManickaP ManickaP merged commit c2b7638 into dotnet:main Nov 22, 2021
@ManickaP ManickaP deleted the mapichov/61505_no_parallel_scavenge branch November 22, 2021 14:07
@karelz karelz added this to the 7.0.0 milestone Nov 23, 2021
@ManickaP
Copy link
Member Author

/backport to release/6.0

@github-actions
Copy link
Contributor

Started backporting to release/6.0: https://github.com/dotnet/runtime/actions/runs/1498779065

@ghost ghost locked as resolved and limited conversation to collaborators Dec 30, 2021
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.

[SslStream] Dispose seems to call blocking IO [HTTP] RemoveStalePools should never run in parallel
4 participants