Closed Kulezi closed 2 years ago
@Kulezi Can I get a little more context of what you did there?
ping @martin-sucha, @zimnx
Please ask for review only when PR is ready to be merged. I usually don't look at the drafts at all.
Made changes with regards to @zimnx comments, also added an integration test that checks if the behavior is correct. Opened issue #271 regarding logs, This PR does not implement any additional wait for responses that reached the driver near contexts deadline, I suppose if it's really necessary it can be added later.
Applied changes suggested by @zimnx.
I also noticed that skipping a single request can increase the inflight request counter in connWriter.loop() even though it wasn't sent, fixed this by changing the behavior from returning nil to returning a custom error in this case. This way the end user will also know the reason request was skipped, and unwrap the underlying context error.
Added missing propagation of request context to compress, now it checks its error too skipping after compressing and before sending.
Added missing context.Background() to _connCloseRequest.
Fixed the StreamID leak, and made the commit messages describe the changes in a more verbose way.
This PR changes NewSession, Session.Prepare, Query.Execute, Query.AsyncExecute to take context.Context as an argument. Context passed to NewSession forces it to close on context cancellation, this is done by:
NewSession's context also stops requests from being sent to server in connWriter.send after cancellation, contexts passed to Session.Prepare, Query.Execute, Query.AsyncExecute provide the same behaviour. Context's cancellation is also checked right before allocating requests StreamID.
Requests that were sent but didn't reach the driver before context cancellation are met with an error response mentioning context cancellation, I'm not sure if this is right or if there should be a bit of additional time to receive the response, as the request may have been sent near context deadline leaving no room for receiving its response.
Also changed OpenConn to use DialContext and HandshakeContext
@martin-sucha @zimnx please let me know if this is the desired behavior, or if I missed something crucial