-
-
Notifications
You must be signed in to change notification settings - Fork 177
Cancelling queries #422
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
Cancelling queries #422
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,6 +1,9 @@ | ||
| package gorethink | ||
|
|
||
| import "encoding/binary" | ||
| import ( | ||
| "encoding/binary" | ||
| "golang.org/x/net/context" | ||
| ) | ||
|
|
||
| // Write 'data' to conn | ||
| func (c *Connection) writeData(data []byte) error { | ||
|
|
@@ -39,3 +42,12 @@ func (c *Connection) writeQuery(token int64, q []byte) error { | |
|
|
||
| return c.writeData(data) | ||
| } | ||
|
|
||
| func (c *Connection) contextFromConnectionOpts() context.Context { | ||
| sum := c.opts.ReadTimeout + c.opts.WriteTimeout | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I am a little worried that this behaviour might be a little unexpected, I think it might make more sense to keep the previous behaviour if no context is passed to a query (I am just trying to think about existing code that might be affected)
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Previous behaviour is a quite useless. If server has continuous queries flow, socket's deadline updates every time so that even first query will not timeout never due to this reason. For me it seems like a bug.
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ok fair enough, I am still a little worried about changing the existing behaviour but hopefully the new behaviour should not cause too many issues. |
||
| if sum == 0 { | ||
| return context.Background() | ||
| } | ||
| ctx, _ := context.WithTimeout(context.Background(), sum) | ||
| return ctx | ||
| } | ||
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 would recommend using the built-in
contextpackage.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.
Most of the libraries use
x/net/context, such as grpc-go, I'm not sure about old version of go, It can be difficult to passx/net/contexttocontextargument.