You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pulsar.apache.org by GitBox <gi...@apache.org> on 2022/01/06 02:38:06 UTC
[GitHub] [pulsar-client-go] wolfstudy opened a new pull request #700: Revert "Use a separate gorutine to handle the logic of reconnect"
wolfstudy opened a new pull request #700:
URL: https://github.com/apache/pulsar-client-go/pull/700
Reverts apache/pulsar-client-go#691
--
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: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar-client-go] bschofield edited a comment on pull request #700: Revert "Use a separate gorutine to handle the logic of reconnect"
Posted by GitBox <gi...@apache.org>.
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()`, 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: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar-client-go] bschofield commented on pull request #700: Revert "Use a separate gorutine to handle the logic of reconnect"
Posted by GitBox <gi...@apache.org>.
bschofield commented on pull request #700:
URL: https://github.com/apache/pulsar-client-go/pull/700#issuecomment-1006589930
If the operations occurring simultaneously could be an issue then it would be easy to add a lock to prevent this.
However I don't think that would prevent the order of operations from being changed?
--
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: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar-client-go] bschofield edited a comment on pull request #700: Revert "Use a separate gorutine to handle the logic of reconnect"
Posted by GitBox <gi...@apache.org>.
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()`, 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?
--
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: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar-client-go] bschofield edited a comment on pull request #700: Revert "Use a separate gorutine to handle the logic of reconnect"
Posted by GitBox <gi...@apache.org>.
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: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar-client-go] wolfstudy merged pull request #700: Revert "Use a separate gorutine to handle the logic of reconnect"
Posted by GitBox <gi...@apache.org>.
wolfstudy merged pull request #700:
URL: https://github.com/apache/pulsar-client-go/pull/700
--
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: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar-client-go] bschofield edited a comment on pull request #700: Revert "Use a separate gorutine to handle the logic of reconnect"
Posted by GitBox <gi...@apache.org>.
bschofield edited a comment on pull request #700:
URL: https://github.com/apache/pulsar-client-go/pull/700#issuecomment-1006589930
If the operations occurring simultaneously could be an issue then it would be easy to add a lock to prevent this.
However I don't think that would prevent the order of operations from being changed from the previous behaviour?
--
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: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar-client-go] bschofield edited a comment on pull request #700: Revert "Use a separate gorutine to handle the logic of reconnect"
Posted by GitBox <gi...@apache.org>.
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?
Also, does it matter that (because of goroutine scheduling) the order in which these operations execute can now be different to before?
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: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar-client-go] bschofield edited a comment on pull request #700: Revert "Use a separate gorutine to handle the logic of reconnect"
Posted by GitBox <gi...@apache.org>.
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()`, 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: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar-client-go] bschofield commented on pull request #700: Revert "Use a separate gorutine to handle the logic of reconnect"
Posted by GitBox <gi...@apache.org>.
bschofield commented 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()`, 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?
--
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: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org