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

Close connections asynchronously and improve gracefulness #6762

Merged

Conversation

JohnMorman
Copy link
Contributor

@JohnMorman JohnMorman commented Sep 29, 2020

Lock around connection cleanup so we don't call .Complete() and .CancelPendingFlush()/.CancelPendingRead() on the same pipe which results in an ObjectDisposedException.

Fixes #6747

EDIT by @ReubenBond:

Updated PR description

  • Add Connection.CloseAsync(Exception) method, which closes the connection asynchronously, completing the returned task once it has
  • Clean up logging around expected termination cases
  • Migrate to newer version of DuplexPipeStream.cs from dotnet/aspnetcore codebase (we are still running a fork until Bedrock is upstreamed into .NET itself or forked into a standalone lib)

{
this.Context.Transport.Output.CancelPendingFlush();
outgoingCompleted = true;
}
Copy link
Member

Choose a reason for hiding this comment

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

I think the result here is that it ends up not gracefully draining the connection before shutdown

@ReubenBond ReubenBond force-pushed the dev/jmorman/ConnectionDisposed branch from c4fa6b3 to 0d2b21a Compare January 8, 2021 23:25
@ReubenBond
Copy link
Member

ReubenBond commented Jan 8, 2021

I pushed an update on top of your commit, @JohnMorman, and closed the PR I had open (#6877)
I also edited the title and description to match

@ReubenBond ReubenBond changed the title Lock around connection cleanup to fix ObjectDisposedException Close connections asynchronously and improve gracefulness Jan 8, 2021
@ReubenBond ReubenBond force-pushed the dev/jmorman/ConnectionDisposed branch from 0d2b21a to e4f9207 Compare January 8, 2021 23:38
@ReubenBond
Copy link
Member

ReubenBond commented Jan 9, 2021

Need to run distributed tests before we can merge this. EDIT: Tests pass cleanly

@benjaminpetit since I made large changes here, could you please take a look, too?

@ReubenBond ReubenBond self-assigned this Jan 9, 2021
JohnMorman and others added 2 commits January 11, 2021 09:12
…elPendingFlush()/.CancelPendingRead() on the same pipe which results in an ObjectDisposedException
@ReubenBond ReubenBond force-pushed the dev/jmorman/ConnectionDisposed branch from e4f9207 to 545e73e Compare January 11, 2021 17:15
@benjaminpetit benjaminpetit merged commit 671d37c into dotnet:master Jan 11, 2021
shmurchi pushed a commit to shmurchi/orleans that referenced this pull request Mar 2, 2021
* Lock around connection cleanup so we don't call .Complete() and .CancelPendingFlush()/.CancelPendingRead() on the same pipe which results in an ObjectDisposedException

* Close connections asynchronously and improve gracefulness

Co-authored-by: ReubenBond <rebond@microsoft.com>
ReubenBond added a commit that referenced this pull request Mar 3, 2021
* Close connections asynchronously and improve gracefulness (#6762)

* Lock around connection cleanup so we don't call .Complete() and .CancelPendingFlush()/.CancelPendingRead() on the same pipe which results in an ObjectDisposedException

* Close connections asynchronously and improve gracefulness

Co-authored-by: ReubenBond <rebond@microsoft.com>

* Adding await for GatewayListNotification

* Updating GatewayManager interfaces

Co-authored-by: John Morman <jmorman@microsoft.com>
Co-authored-by: ReubenBond <rebond@microsoft.com>
@github-actions github-actions bot locked and limited conversation to collaborators Dec 4, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ObjectDisposedException in Connection.cs on Silo Shutdown
3 participants