producer: Integrate context.Context with publishing #365
Conversation
producer.go
Outdated
| // keep the connection alive if related to context timeout | ||
| // need to do some stuff that's in Producer.popTransaction here | ||
| w.transactions = w.transactions[1:] | ||
| t.Error = err |
There was a problem hiding this comment.
I don't love this code we've copied in to this context-specific edge case, can't we consolidate this in to popTransaction? Do we need to define a context-specific error code for the protocol?
There was a problem hiding this comment.
Yeah I wasn't a huge fan of how that was being handled...
I updated the code to invoke popTransaction without modifying the function signature, meaning I seemingly need to have at least one new "frameType" in protocol.go to handle the context errors. I opted to be explicit and add two "frameTypes" (for context.Cancelled and context.DeadlineExceeded respectively) so that we would not need to infer the context error from the byte slice/handle a default case like so:
if frameType == FrameTypeContextError {
switch string(data) {
case context.Canceled.Error():
t.Error = context.Canceled
case context.DeadlineExceeded.Error():
t.Error = context.DeadlineExceeded
default:
t.Error = ???
}
}The FrameTypeContextCanceled and FrameTypeContextDeadlineExceeded are not really "frameTypes" per se, so I also don't fully love this approach, but it feels better than what it was.
I'm curious to hear your thoughts on this approach and any suggestions you may have on how to improve upon this.
There was a problem hiding this comment.
Can popTransaction take a 3rd error argument so we don't use magic values for some errors?
05a200f to
7ba56c7
Compare
|
@mreiferson thanks for the initial review! Apologies it took so long for me to circle back around to this, but it should be good for another look 👍 |
|
Seems exactly like what I need, thanks! I've reviewed the pull request and it looks good to me! |
Merging current content of nsqio#365
| return w.PingWithContext(ctx) | ||
| } | ||
|
|
||
| func (w *Producer) PingWithContext(ctx context.Context) error { |
There was a problem hiding this comment.
We need to duplicate all of the docstrings onto the new methods
producer.go
Outdated
| // keep the connection alive if related to context timeout | ||
| // need to do some stuff that's in Producer.popTransaction here | ||
| w.transactions = w.transactions[1:] | ||
| t.Error = err |
There was a problem hiding this comment.
Can popTransaction take a 3rd error argument so we don't use magic values for some errors?
This PR sets out to resolve #360 with the integration of context.Context in the
nsq.Producer. In order to maintain backwards compatibility, new methods were added that have names with the suffixWithContext. These new methods mostly correspond to those that a client would use to publish messages and they adhere to context timeouts/deadlines. Once the deadline has been reached, an error will be returned to the client and the givenProducerTransactionwill be dropped/not published, all while keeping the connection tonsqdalive.An overview of the new methods added are below:
nsq.Conn
nsq.Producer
The idea here being that these
WithContextmethods would essentially replace the existing ones eventually, which would be a breaking/major version change forgo-nsq.