search

wistia / nsq-ruby

NSQ Ruby client
MIT License
68 stars 27 forks source link

Ensure write_loop ends; pass message via write_queue #32

Closed alieander closed 7 years ago

alieander commented 8 years ago

Why?

There is a race condition with the current implementation where by setting @stop_write_loop = true can potentially not be seen by the Thread running the write_loop. Even with a Mutex around the variable @stop_write_loop, there is a chance that the .join(1) will enter the loop and imediately call @write_queue.pop. If there is nothing in the queue at the time pop is called, the thread will block waiting for new data. In the case that we are closeing the current connection, this thread will be left for garbage collection at some later date...

Change

The change here removes that variable @stop_write_loop and instead, pushes a symbol :stop_write_loop onto the write_queue for the write_loop to see and then act on.

Potential issues

The with_shaky_connection test seems to fail for me inconsistently... This potentially points to something I have overlooked, however when snooze is allowed to run as normal I do not see it fail.

I have done quite a bit of empirical work with this branch and have yet to see data lose or reason for concern.

Other thoughts

There might be a cleaner solution in Queue.close as you could close the queue and simply allow that to be the indicator of done-ness but that is new to ruby 2.3 so I thought it might be a little restrictive for those not yet up to that release.

ghost commented 8 years ago

@alieander, thanks for your PR! By analyzing the annotation information on this pull request, we identified @bschwartz, @freerobby and @Soulou to be potential reviewers

alieander commented 8 years ago

I did some further testing and as far as I can tell, the testing issue with stubbing snooze and with_shaky_connection is very likely do to it going getting to 100 retries much faster now.

Without having some attempts get blocked for the .join(1) 1 second timeout, it retries much faster...

Any chance someone might have a chance to review this?

freerobby commented 8 years ago

Thanks for this! Sorry for the delay, I've been a bit swamped and didn't have enough time to give this as thorough a review as it merits. I'll try to get to it by the end of the weekend. Thanks for bearing with!

Sent from my iPhone

On Nov 10, 2016, at 12:40 PM, alieander notifications@github.com wrote:

I did some further testing and as far as I can tell, the testing issue with stubbing snooze and with_shaky_connection is very likely do to it going getting to 100 retries much faster now.

Without having some attempts get blocked for the .join(1) 1 second timeout, it retries much faster...

Any chance someone might have a chance to review this?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.

leklund commented 7 years ago

I was doing some perf testing and pretty sure this is what causing some thread leakage. Any chance it can be reviewed and merged? Anything I can do to help?

freerobby commented 7 years ago

@leklund Thanks -- I didn't get a chance to test this as thoroughly as I'd hoped, but I did not find any problems in what I did. Going to merge given this seems like an improvement and no reports of problems from anyone who's tried it.

alieander commented 7 years ago

Thanks a bunch!

alieander commented 7 years ago

@freerobby hate to be a bother but when do you normally push new releases for gems?

freerobby commented 7 years ago

@alieander Generally after a merge and as soon as I get out of meetings and am back at my computer. :)

Will do shortly!

On Mon, Nov 21, 2016 at 4:00 PM, alieander notifications@github.com wrote:

@freerobby https://github.com/freerobby hate to be a bother but when do you normally push new releases for gems?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/wistia/nsq-ruby/pull/32#issuecomment-262065574, or mute the thread https://github.com/notifications/unsubscribe-auth/AAHEckgCTja1SmEBQ7cWhpi3FzjpKl2Dks5rAgZogaJpZM4KoB8c .

Robby Grossman Director of Engineering Wistia

alieander commented 7 years ago

Thanks you so much! Sorry to be a bother ...

freerobby commented 7 years ago

You are not a bother at all!! Thank you for contributing and putting up with my tardiness!

On Mon, Nov 21, 2016 at 4:13 PM, alieander notifications@github.com wrote:

Thanks you so much! Sorry to be a bother ...

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/wistia/nsq-ruby/pull/32#issuecomment-262068963, or mute the thread https://github.com/notifications/unsubscribe-auth/AAHEchRlvy3yk18Kmxp8pZGWbX3L5UaNks5rAgmJgaJpZM4KoB8c .

Robby Grossman Director of Engineering Wistia

alieander commented 7 years ago

... Poke ? ... Sorry to be mean but could we get a gem push today?

freerobby commented 7 years ago

@alieander I pushed! https://rubygems.org/gems/nsq-ruby :)

On Tue, Nov 22, 2016 at 4:57 PM, alieander notifications@github.com wrote:

... Poke ? ... Sorry to be mean but could we get a gem push today?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/wistia/nsq-ruby/pull/32#issuecomment-262378017, or mute the thread https://github.com/notifications/unsubscribe-auth/AAHEcn79hc6gy6XyGnN-QxBczCmnyjC9ks5rA2VWgaJpZM4KoB8c .

Robby Grossman Director of Engineering Wistia

alieander commented 7 years ago

Thanks so much!

freerobby commented 7 years ago

Sorry, should have replied here when I did that. 2.0.3 has your latest merged PR.

On Tue, Nov 22, 2016 at 4:58 PM, Robby Grossman robby@wistia.com wrote:

@alieander I pushed! https://rubygems.org/gems/nsq-ruby :)

On Tue, Nov 22, 2016 at 4:57 PM, alieander notifications@github.com wrote:

... Poke ? ... Sorry to be mean but could we get a gem push today?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/wistia/nsq-ruby/pull/32#issuecomment-262378017, or mute the thread https://github.com/notifications/unsubscribe-auth/AAHEcn79hc6gy6XyGnN-QxBczCmnyjC9ks5rA2VWgaJpZM4KoB8c .

Robby Grossman Director of Engineering Wistia

Robby Grossman Director of Engineering Wistia