producer: Integrate context.Context with publishing

[Ulminator]

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 suffix WithContext. 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 given ProducerTransaction will be dropped/not published, all while keeping the connection to nsqd alive.

An overview of the new methods added are below:

nsq.Conn

• ConnectWithContext
• WriteCommandWithContext

nsq.Producer

• PingWithContext
• PublishWithContext
• MultiPublishWithContext
• DeferredPublishWithContext
• PublishAsyncWithContext
• MultiPublishAsyncWithContext
• DeferredPublishAsyncWithContext

The idea here being that these WithContext methods would essentially replace the existing ones eventually, which would be a breaking/major version change for go-nsq.

[mreiferson]

Thanks for this, a few minor comments. I don't have any better ideas on naming functions in backwards-compatible way.

[mreiferson]

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?

[Ulminator]

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.

[jehiah]

Can popTransaction take a 3rd error argument so we don't use magic values for some errors?

[Ulminator]

@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 👍

[martin-sucha]

Seems exactly like what I need, thanks!

I've reviewed the pull request and it looks good to me!

[jehiah]

We need to duplicate all of the docstrings onto the new methods

[jehiah]

Can popTransaction take a 3rd error argument so we don't use magic values for some errors?

[Ulminator]

Hey @jehiah ,

Can popTransaction take a 3rd error argument so we don't use magic values for some errors?

I updated popTransaction to support this so now when there is a context error it is invoked like so:

w.popTransaction(-1, []byte{}, err)

I set the first arg (frameType) to an invalid one since the only options are the following:

const (
	FrameTypeResponse int32 = 0
	FrameTypeError    int32 = 1
	FrameTypeMessage  int32 = 2
)

Please let me know if I should change anything