-
Notifications
You must be signed in to change notification settings - Fork 103
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
Graceful shutdown controlling process. #709
Conversation
def964a
to
655b5a6
Compare
ae2120d
to
ed3784a
Compare
src/Microsoft.Azure.SignalR.Common/Interfaces/IServiceConnection.cs
Outdated
Show resolved
Hide resolved
...Microsoft.Azure.SignalR.Common/ServiceConnections/MultiEndpointServiceConnectionContainer.cs
Outdated
Show resolved
Hide resolved
src/Microsoft.Azure.SignalR.Common/ServiceConnections/StrongServiceConnectionContainer.cs
Outdated
Show resolved
Hide resolved
src/Microsoft.Azure.SignalR.Common/ServiceConnections/WeakServiceConnectionContainer.cs
Show resolved
Hide resolved
src/Microsoft.Azure.SignalR.Common/ServiceConnections/ServiceConnectionContainerBase.cs
Outdated
Show resolved
Hide resolved
4709f0d
to
7944265
Compare
src/Microsoft.Azure.SignalR.Common/Interfaces/IServiceConnection.cs
Outdated
Show resolved
Hide resolved
In my opinions:
|
@vwxyzh In another word. |
0b3e83e
to
94ebdba
Compare
94ebdba
to
e7920bb
Compare
src/Microsoft.Azure.SignalR.Common/ServiceConnections/ServiceConnectionBase.cs
Outdated
Show resolved
Hide resolved
23f4ebe
to
045c4b7
Compare
045c4b7
to
4061bf4
Compare
src/Microsoft.Azure.SignalR.Common/ServiceConnections/StrongServiceConnectionContainer.cs
Outdated
Show resolved
Hide resolved
13e2ed7
to
aea9317
Compare
src/Microsoft.Azure.SignalR.Common/ServiceConnections/ServiceConnectionContainerBase.cs
Outdated
Show resolved
Hide resolved
src/Microsoft.Azure.SignalR.Common/ServiceConnections/ServiceConnectionContainerBase.cs
Outdated
Show resolved
Hide resolved
src/Microsoft.Azure.SignalR.Common/ServiceConnections/ServiceConnectionContainerBase.cs
Show resolved
Hide resolved
9ad5e6f
to
d4d4dd7
Compare
{ | ||
public ServiceConnectionStatus Status { get; set; } | ||
|
||
public Task ConnectionInitializedTask => Task.Delay(TimeSpan.FromSeconds(1)); |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
d4d4dd7
to
08cea5b
Compare
Split from #689
Contain only
ControllingProcess
for graceful shutdown, a few newConstants
and 2 newOptions
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 allEndpoints
after receiving a message. In my current workflow, I shutdown theEndpoints
respectively, which means if all client connections fromEndpoint A
have been dropped, the relevant server connection will be dropped immediately. And if our SDK received a message fromEndpoint B
, it wouldn't have any chance to broadcast messages toEndpoint A
.So maybe I have to introduce a barrier to handle this situation, and the workflow may like this:
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:
The
CloseAsync
method will be implemented in the main PR.