michaeljmarshall commented on a change in pull request #7355:
URL: https://github.com/apache/pulsar/pull/7355#discussion_r639953231
##########
File path:
pulsar-broker/src/main/java/org/apache/pulsar/broker/service/persistent/PersistentTopic.java
##########
@@ -688,12 +688,13 @@ public void openCursorFailed(ManagedLedgerException
exception, Object ctx) {
private CompletableFuture<? extends Subscription>
getNonDurableSubscription(String subscriptionName,
MessageId startMessageId, long startMessageRollbackDurationSec) {
- CompletableFuture<Subscription> subscriptionFuture = new
CompletableFuture<>();
log.info("[{}][{}] Creating non-durable subscription at msg id {}",
topic, subscriptionName, startMessageId);
synchronized (ledger) {
// Create a new non-durable cursor only for the first consumer
that connects
- Subscription subscription =
subscriptions.computeIfAbsent(subscriptionName, name -> {
Review comment:
@merlimat - I know this has been merged for a while, but I was reading
through this class today, and I am concerned about a possible race condition
that would lead to an incorrect update to the `subscriptions` map.
With this PR, the `subscriptions` map is no longer updated using the
`computeIfAbsent` method here, but is instead modified with a `get` and then a
subsequent `put`. The `getDurableSubscription` updates the `subscriptions` map
concurrently as well. I believe this opens us up to a race under the following
conditions:
1. We call get for a subscription on line 695, and get `null`.
2. Then, a durable subscription with the same name is added to the
`subscriptions` map on line 665.
3. Then, the non durable subscription is put into the `subscriptions` map on
new line 722, and in doing so, overwrites the durable subscription of the same
name.
Do you agree this is a risk here? If so, I will work on contributing a fix.
I wanted to check first though, just in case there is context I'm missing.
Thanks!
--
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.
For queries about this service, please contact Infrastructure at:
[email protected]
