-
Notifications
You must be signed in to change notification settings - Fork 297
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
rpcclient: Use passed contexts over websocket connections. #2198
Conversation
d952fc6
to
f1067f5
Compare
Can write tests if needed. |
ce45637
to
0d483f5
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.
The updates look good overall. I've left some inline comments that address a few things, including a bug.
This will also need a rebase over master as it conflicts with #2205. |
The passed context was ignored until a reconnect. This kills the client when the passed context is done. This is much more useful for consumers who need the client to shut down when their context if finished.
are |
Both |
Store the initial context in result types and stop waiting on context done with ErrRequestCanceled by giving all result types a context field and having receiveFuture stop waiting on context done with an ErrRequestCanceled. Result types have been changed from a channel of *response to a new cmdRes type that can hold the context. Remove duplicate function futureError and unused sendCmdAndWait.
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.
Code looks good to go now. I'm going to give it a thorough testing before approval.
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.
I've tested this pretty well and everything seems to be working as expected.
In addition to just general testing in the normal positive paths, I particularly focused testing on context cancellation at various points as follows:
- Before the call has been sent to the server
- After the call has been sent to the server, but before the response has been received
- After the response has been received from the server
One thing to keep in mind is that because the async receive machinery is selecting across the context and the response channel and Go has fair random channel selection, it is a coin flip as to whether or not the call to Receive
will succeed in the case the context is cancelled when the response has already been received from the server.
I personally think this is both acceptable and appropriate behavior since the caller really shouldn't be trying to read a response after cancelling the context anyway, but I thought it was worth mentioning since any addition of tests around this behavior would need to account for that possibility.
rpcclient: Stop client on ctx done.
The passed context was ignored until a reconnect. This kills the client
when the passed context is done. This is much more useful for consumers
who need the clien to shut down when their context if finished.
rpcclient: Add a lifetime to requests.
Requests over websocket would block indefintely waiting for a response.
This adds a way for the caller to cancel requests, such as using a
context with deadline.