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

Graceful shutdown controlling process. #709

Merged

Conversation

terencefan
Copy link
Member

@terencefan terencefan commented Oct 29, 2019

Split from #689

Contain only ControllingProcess for graceful shutdown, a few new Constants and 2 new Options

UPDATE 11/1

@vicancy @KKhurin @vwxyzh

After discussed with @chenkennt, we found a barrier between all client connection closed and close server connections is necessary.

Considering if our SDK connected with multiple Endpoints, and it would like to broadcast to all Endpoints after receiving a message. In my current workflow, I shutdown the Endpoints respectively, which means if all client connections from Endpoint A have been dropped, the relevant server connection will be dropped immediately. And if our SDK received a message from Endpoint B, it wouldn't have any chance to broadcast messages to Endpoint A.

So maybe I have to introduce a barrier to handle this situation, and the workflow may like this:

image

By doing this, we can make sure our server connections to each endpoint will stay alive until even our SDK has received the last message from its last client connection.

Is this useful? Please give me your advice.

And here is the overall process:

image

The CloseAsync method will be implemented in the main PR.

@terencefan terencefan force-pushed the graceful-shutdown-controlling-process branch 2 times, most recently from def964a to 655b5a6 Compare October 29, 2019 10:13
@terencefan terencefan requested a review from vicancy October 29, 2019 10:18
@terencefan terencefan force-pushed the graceful-shutdown-controlling-process branch from ae2120d to ed3784a Compare October 29, 2019 10:19
@terencefan terencefan force-pushed the graceful-shutdown-controlling-process branch 4 times, most recently from 4709f0d to 7944265 Compare October 31, 2019 08:40
@vwxyzh
Copy link
Contributor

vwxyzh commented Nov 1, 2019

In my opinions:

@terencefan
Copy link
Member Author

terencefan commented Nov 1, 2019

@vwxyzh
There are still small differences between these 2 methods,
For StopAsync, we cancel the pending messages in the pipe.
For CloseAsync, we simply send a message to our runtime, but nothing to do with our pipes.

In another word. StopAsync close server connection positively on the SDK side, while CloseAsync just tells our ASRS and wait for the connection to be closed passively.

@terencefan terencefan force-pushed the graceful-shutdown-controlling-process branch 7 times, most recently from 0b3e83e to 94ebdba Compare November 5, 2019 03:40
@terencefan terencefan force-pushed the graceful-shutdown-controlling-process branch from 94ebdba to e7920bb Compare November 5, 2019 05:07
@terencefan terencefan force-pushed the graceful-shutdown-controlling-process branch 3 times, most recently from 23f4ebe to 045c4b7 Compare November 5, 2019 05:18
@terencefan terencefan force-pushed the graceful-shutdown-controlling-process branch from 045c4b7 to 4061bf4 Compare November 5, 2019 05:23
@terencefan terencefan force-pushed the graceful-shutdown-controlling-process branch 5 times, most recently from 13e2ed7 to aea9317 Compare November 5, 2019 06:02
@terencefan terencefan force-pushed the graceful-shutdown-controlling-process branch 3 times, most recently from 9ad5e6f to d4d4dd7 Compare November 5, 2019 09:05
{
public ServiceConnectionStatus Status { get; set; }

public Task ConnectionInitializedTask => Task.Delay(TimeSpan.FromSeconds(1));
Copy link
Member

Choose a reason for hiding this comment

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

Delay 1 sec is too long, maybe 10 milliseconds?

Copy link
Member Author

Choose a reason for hiding this comment

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

Actually we don't have to wait in the current implementation since our StartCoreAsync doesn't truly await anything before calling inner StartAsync.

But what if it does something that needs to be awaited, and, won't cost too long in future implementation? (like write a log to file, cause real await but very short)

That's why I introduced a 1s delay in this test case. 1s for 1 test case is fair, to strike a balance between robust and time-costs.

@terencefan terencefan force-pushed the graceful-shutdown-controlling-process branch from d4d4dd7 to 08cea5b Compare November 5, 2019 09:52
@terencefan terencefan merged commit 26bfb99 into Azure:dev Nov 5, 2019
@terencefan terencefan deleted the graceful-shutdown-controlling-process branch November 5, 2019 10:02
JialinXin pushed a commit to JialinXin/azure-signalr that referenced this pull request Dec 20, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants