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

Handle graceful shutdown of ConsumerWorkPool when canceling a consumer #437

Merged
merged 4 commits into from Oct 11, 2016

Conversation

Projects
None yet
2 participants
@stefansedich
Contributor

stefansedich commented Oct 7, 2016

This carries on the work of #341 to implement this feature fully. You can now configure a worker pool shutdown timeout when creating the channel (off by default).

Then upon processing of the basic_cancel instead of killing the worker pool as previously done, the worker pool will be shut down and instructed to wait for any workers to complete (within the timeout period).

This allows for a consumer to stop receiving new messages but still be able to complete any existing work before the worker pool is shut down.

All existing behavior remains in the case of closing a connection, we will not wait for workers as the worker might need to ack the message and in this scenario the connection has been closed and this would not be possible.

I will update the documentation as a seperate PR.

Show outdated Hide outdated lib/bunny/consumer_work_pool.rb
@@ -17,9 +17,12 @@ class ConsumerWorkPool
attr_reader :size
attr_reader :abort_on_exception
def initialize(size = 1, abort_on_exception = false)
def initialize(size = 1, abort_on_exception = false, shutdown_timeout = nil)

This comment has been minimized.

@michaelklishin

michaelklishin Oct 7, 2016

Member

I think it's perfectly fine to have a non-nil default timeout.

@michaelklishin

michaelklishin Oct 7, 2016

Member

I think it's perfectly fine to have a non-nil default timeout.

Show outdated Hide outdated lib/bunny/session.rb
@@ -337,14 +337,14 @@ def transport_write_timeout
# opened (this operation is very fast and inexpensive).
#
# @return [Bunny::Channel] Newly opened channel
def create_channel(n = nil, consumer_pool_size = 1, consumer_pool_abort_on_exception = false)
def create_channel(n = nil, consumer_pool_size = 1, consumer_pool_abort_on_exception = false, consumer_pool_shutdown_timeout = nil)

This comment has been minimized.

@michaelklishin

michaelklishin Oct 7, 2016

Member

Again, a non-nil timeout (maybe up to 60 seconds?) sounds OK and would make most people benefit from this improvement.

@michaelklishin

michaelklishin Oct 7, 2016

Member

Again, a non-nil timeout (maybe up to 60 seconds?) sounds OK and would make most people benefit from this improvement.

This comment has been minimized.

@stefansedich

stefansedich Oct 7, 2016

Contributor

Ok sounds good, I wasn't sure so left it off but I think this makes sense :)

@stefansedich

stefansedich Oct 7, 2016

Contributor

Ok sounds good, I wasn't sure so left it off but I think this makes sense :)

@michaelklishin

This comment has been minimized.

Show comment
Hide comment
@michaelklishin

michaelklishin Oct 7, 2016

Member

It looks good at first glance. I'd recommend using a timeout by default, e.g. the same the Java client uses.

Member

michaelklishin commented Oct 7, 2016

It looks good at first glance. I'd recommend using a timeout by default, e.g. the same the Java client uses.

@stefansedich

This comment has been minimized.

Show comment
Hide comment
@stefansedich

stefansedich Oct 7, 2016

Contributor

Changed to use a 60s timeout as mentioned, I wanted to not change behavior but after you mentioned it I think it makes sense to be safe by default.

Contributor

stefansedich commented Oct 7, 2016

Changed to use a 60s timeout as mentioned, I wanted to not change behavior but after you mentioned it I think it makes sense to be safe by default.

@stefansedich

This comment has been minimized.

Show comment
Hide comment
@stefansedich

stefansedich Oct 11, 2016

Contributor

@michaelklishin any more thoughts?

Contributor

stefansedich commented Oct 11, 2016

@michaelklishin any more thoughts?

@michaelklishin michaelklishin merged commit 661b4c8 into ruby-amqp:master Oct 11, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@michaelklishin

This comment has been minimized.

Show comment
Hide comment
@michaelklishin

michaelklishin Oct 11, 2016

Member

Thank you.

Member

michaelklishin commented Oct 11, 2016

Thank you.

@stefansedich

This comment has been minimized.

Show comment
Hide comment
@stefansedich

stefansedich Oct 11, 2016

Contributor

Thanks, I will update the documentation when I get a spare minute through the week.

Contributor

stefansedich commented Oct 11, 2016

Thanks, I will update the documentation when I get a spare minute through the week.

eebs added a commit to eebs/bunny-mock that referenced this pull request Dec 19, 2016

Allow Session#create_channel to accept additional args
In ruby-amqp/bunny#382 and ruby-amqp/bunny#437 `Session#create_channel` got some additional arguments. This change allows calling of `create_channel` with additional arguments.

eebs added a commit to eebs/bunny-mock that referenced this pull request Dec 19, 2016

Allow Session#create_channel to accept additional args
In ruby-amqp/bunny#382 and
ruby-amqp/bunny#437 `Session#create_channel` got
some additional arguments. This change allows calling of
`create_channel` with additional arguments.

arempe93 added a commit to arempe93/bunny-mock that referenced this pull request Apr 10, 2017

Allow Session#create_channel to accept additional args
In ruby-amqp/bunny#382 and
ruby-amqp/bunny#437 `Session#create_channel` got
some additional arguments. This change allows calling of
`create_channel` with additional arguments.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment