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 2021/05/13 23:52:33 UTC
[GitHub] [pulsar] devinbost opened a new pull request #10583: Synchronize OpAddEntry.safeRun() to prevent race on updating lastConfirmedEntry
devinbost opened a new pull request #10583:
URL: https://github.com/apache/pulsar/pull/10583
I noticed that OpAddEntry.safeRun() is modifying a volatile field (`ml.lastConfirmedEntry`) here: https://github.com/apache/pulsar/blob/55d6a77ec8e7d05f00f906111b7de63d6b52c74a/managed-ledger/src/main/java/org/apache/bookkeeper/mledger/impl/OpAddEntry.java#L197
Since we don't have a mutex while updating `lastConfirmedEntry`, it looks like that could create a concurrency issue if another thread races on the update.
I found this while investigating #6054, though testing confirmed that this PR doesn't prevent the `lastConfirmedEntry` from getting stuck in that issue (as reported in this comment: https://github.com/apache/pulsar/issues/6054#issuecomment-840894974).
--
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:
users@infra.apache.org
[GitHub] [pulsar] devinbost commented on pull request #10583: Synchronize OpAddEntry.safeRun() to prevent race on updating lastConfirmedEntry
Posted by GitBox <gi...@apache.org>.
devinbost commented on pull request #10583:
URL: https://github.com/apache/pulsar/pull/10583#issuecomment-840899701
Please review @codelipenghui @rdhabalia @lhotari
--
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:
users@infra.apache.org
[GitHub] [pulsar] skyrocknroll commented on pull request #10583: Synchronize OpAddEntry.safeRun() to prevent race on updating lastConfirmedEntry
Posted by GitBox <gi...@apache.org>.
skyrocknroll commented on pull request #10583:
URL: https://github.com/apache/pulsar/pull/10583#issuecomment-840924787
@merlimat As I have mentioned earlier, if two threads are both reading and writing to a shared variable, then using the volatile keyword for that is not enough. You need to use a synchronized in that case to guarantee that the reading and writing of the variable is atomic. Reading or writing a volatile variable does not block threads reading or writing. For this to happen you must use the synchronized keyword around critical sections.
As an alternative to a synchronized block you could also use one of the many atomic data types found in the java.util.concurrent package. For instance, the AtomicLong or AtomicReference or one of the others.
In case only one thread reads and writes the value of a volatile variable and other threads only read the variable, then the reading threads are guaranteed to see the latest value written to the volatile variable. Without making the variable volatile, this would not be guaranteed.
Ref http://tutorials.jenkov.com/java-concurrency/volatile.html
--
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:
users@infra.apache.org
[GitHub] [pulsar] merlimat commented on pull request #10583: Synchronize OpAddEntry.safeRun() to prevent race on updating lastConfirmedEntry
Posted by GitBox <gi...@apache.org>.
merlimat commented on pull request #10583:
URL: https://github.com/apache/pulsar/pull/10583#issuecomment-840916995
> Since we don't have a mutex while updating lastConfirmedEntry, it looks like that could create a concurrency issue if another thread races on the update.
Since the field is `volatile`, the update will be immediately visible to any other thread reading that variable. Where you suggest that the race would be?
--
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:
users@infra.apache.org
[GitHub] [pulsar] merlimat commented on pull request #10583: Synchronize OpAddEntry.safeRun() to prevent race on updating lastConfirmedEntry
Posted by GitBox <gi...@apache.org>.
merlimat commented on pull request #10583:
URL: https://github.com/apache/pulsar/pull/10583#issuecomment-840944024
> As I have mentioned earlier, if two threads are both reading and writing to a shared variable, then using the volatile keyword for that is not enough. You need to use a synchronized in that case to guarantee that the reading and writing of the variable is atomic. Reading or writing a volatile variable does not block threads reading or writing. For this to happen you must use the synchronized keyword around critical sections.
The `volatile` qualifier will guarantee that other threads will read the latest value of that variable. Also note that in this case there's just 1 single thread writing it.
We don't have code in there (for this variable) that reads the value, does something and writes it back.
The only addition that `AtomicReference` would give is to do `compareAndSet()` on the variable reference. But we're not needing it for `lastConfirmedEntry`
--
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:
users@infra.apache.org
[GitHub] [pulsar] devinbost closed pull request #10583: Synchronize OpAddEntry.safeRun() to prevent race on updating lastConfirmedEntry
Posted by GitBox <gi...@apache.org>.
devinbost closed pull request #10583:
URL: https://github.com/apache/pulsar/pull/10583
--
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:
users@infra.apache.org
[GitHub] [pulsar] devinbost commented on pull request #10583: Synchronize OpAddEntry.safeRun() to prevent race on updating lastConfirmedEntry
Posted by GitBox <gi...@apache.org>.
devinbost commented on pull request #10583:
URL: https://github.com/apache/pulsar/pull/10583#issuecomment-840976438
@merlimat I took another look at the code, and you're right. I'll close this PR.
--
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:
users@infra.apache.org