Fix #275 #276
Fix #275 #276
Conversation
Signed-off-by: Paul MARS <[email protected]>
Signed-off-by: Paul MARS <[email protected]>
Signed-off-by: Paul MARS <[email protected]>
|
|
||
| func TestPublishSubscribe_do_not_block_without_ack_required(t *testing.T) { |
There was a problem hiding this comment.
Can we use a small function name to make it more concise?
|
Please resolve the test :) |
| s.logger.Trace("Sent message to subscriber without ack", logFields) | ||
| return | ||
| } | ||
|
|
There was a problem hiding this comment.
The select below isn't really blocking execution, as it's running in another goroutine. BlockPublishUntilSubscriberAck is checked in Publish and that's where it blocks.
Can you share more about your use case?
There was a problem hiding this comment.
@m110 the current implementation will cause deadlock on all subscribers even if one of them did Ack/Nack a message. However I believe only the subscriber that does not Ack/Nack should lose the message leaving the rest processing goes through. So the setting to block publishing will deadlock if one subscriber does not Ack and I don't think that the intention.
There was a problem hiding this comment.
My suggestion will be to timeout when the BlockPublishUntilSubscriberAck is set to avoid the deadlock. That means when an Ack/Nack is not received after some time for a given subscriber the message is lost because the timeout will kick in and the context will be cancelled.
This PR contains a fix for the issue but for now other PubSub tests are broken (eg. TestResendOnError).
I may have misunderstood how the Ack/Nack mechanism is supposed to work.
@roblaszczak @m110 I would love to discuss it further if you have some time.