bschofield edited a comment on pull request #700:
URL: https://github.com/apache/pulsar-client-go/pull/700#issuecomment-1006570365
Just to be clear, this commit doesn't actually revert #691 but instead adds
a close channel, right? If so it might be good to update the title.
I do have a separate concern with this approach, which might or might not be
justified. It looks to me like operations which were previously serialized by
virtue of the single goroutine in `runEventsLoop()` can now occur concurrently,
because they are now running in separate goroutines.
For example: suppose the user calls `p.Flush()` on the producer, which
causes a `&flushRequest{}` to be enqueued to `p.eventsChan`. The primary
goroutine in `runEventsLoop()` will then call `p.internalFlush()` which will
try to write to `p.cnx`. Suppose that at the same time, the broker closes the
connection. and the new goroutine calls `p.reconnectToBroker()`, which tries to
change `p.cnx` whilst `p.internalFlush()` is still using it.
Does it not matter that `p.reconnectToBroker()` can now be happening at the
same time as `p.internalSend()` / `p.internalFlush()` / `p.internalClose() /
p.internalFlushCurrentBatch()`, when previously the use of a single goroutine
meant that these functions could not be executing simultaneously? Is there some
other lock or feature of the code that means this is OK? Or does this introduce
a race condition?
Would really appreciate your thoughts @wolfstudy / @cckellogg. Apologies in
advance if you already considered this possibility and there is no issue.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
