-
Notifications
You must be signed in to change notification settings - Fork 288
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
multi: Convert rpcserver lifecycle to context. #2043
Conversation
483807d
to
0e19707
Compare
0e19707
to
ac185e7
Compare
ac185e7
to
7b92c0c
Compare
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.
Seems fine now!
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.
The overall approach looks good with the most recent iteration. Nice work so far. I did mention a final area for improvement in regards to the notifications with the inline comments as well as a couple of other very minor things.
I also manually tested the changes I recommended to the notifications via various RPC clients and the race detector to ensure the suggestion works as intended.
Finally, I gave this a thorough review and verified that there are no more call sites that are attempting to send on channels serviced by goroutines spawned by the RPC server and notification manager which aren't ultimately selecting across a channel with the context which could lead to a hang if the goroutines were to disappear out from under it.
7b92c0c
to
e8e7428
Compare
This updates the rpcserver and wsNotificationManager lifecycle processes to use a context.
e8e7428
to
fc57c94
Compare
This updates the
rpcserver
andwsNotificationManager
lifecycle processes to use a context.Resolves #2037