wistia / nsq-ruby

NSQ Ruby client
MIT License
68 stars 27 forks source link

Synchronous producer - Handle acknowledgements of sent PUB/MPUB/DPUB messages #52

Open Soulou opened 5 years ago

Soulou commented 5 years ago

The goal of this PR is to update the library to ensure that an exception is raised if an error happens when a message is sent to NSQd.

This is a work in progress, all the inputs are welcomed.

Related #51

Soulou commented 5 years ago

The first step has been to gather read/write operations in a single thread in order to share information between both operations in an easier way.

Soulou commented 5 years ago

Second step, not sure it's the best implementation but a SizableQueue of 1 item is created for each message and publishing Thread is waiting for the response to be received. I got inspired from the go implementation which is handling it this way.

Soulou commented 5 years ago

I think the PR is ready, the API hasn't changed, the behavior either, but the internals have slightly been updated as there is only one read/write loop based on a select.

The code is ready for review. All the old specs are passing correctly.

Soulou commented 5 years ago

@bschwartz a small follow up to get your input on this.

Soulou commented 5 years ago

There is a race condition on the failing tests.

if SUB is sent too early in nsq starting process, it is refused with E_INVALID invalid SUB during init state As it's not detected, the consumer is not registering on the topic and not getting the message.

The bug exists on master too, all the other specs are passing

bschwartz commented 5 years ago

This is looking good @Soulou. I should have time Friday for a more in depth review. In the meantime, do you think you could also update the README to explain the new functionality?

Also, if you have some ideas of how to fix the spec issue that you mentioned also exists on master, that would be great!

Soulou commented 5 years ago

Thanks @bschwartz I'll update the README and try to fix the spec,

Soulou commented 5 years ago

For info, this PR is already running in our production environment, so far so good.

Soulou commented 5 years ago

@bschwartz done :)