Skip to content
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

producer: Integrate context.Context with publishing #365

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

Ulminator
Copy link

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.

Copy link
Member

@mreiferson mreiferson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

c.mtx.Unlock()
return ctx.Err()
default:
_, err := cmd.WriteTo(c)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we're shadowing err here and it won't be correct on line 323

// 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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

*: context.Context and timeouts
3 participants