visit2rahul commented on PR #4487:
URL: https://github.com/apache/polaris/pull/4487#issuecomment-4500251297
Thank you @nandorKollar and @adutra for thinking this through carefully.
On drawback: there is none meaningful. The performance cost on modern JVMs
is essentially zero (re-entrant acquisition of an already-held monitor is
JIT-erased via biased locking), and behavior is unchanged at the current
smallrye version.
One additional angle worth surfacing: this also affects future callers. If
we (or a downstream extension) ever add another call site that invokes
processor.onNext() without external synchronization, the bug surfaces silently
the moment smallrye changes. Having synchronized(processor) { onNext(...) }
established at the existing call site makes it more likely any new caller
follows the same pattern - it shifts the burden from "remember to synchronize"
to "follow the established pattern at this listener."
So between:
- Keeping docs-only: external synchronized is technically redundant today;
relies on the in-code comment to inform future readers.
- Restoring the synchronized: zero runtime cost, defends against future
smallrye changes AND establishes the synchronized-access pattern for any future
caller.
I am happy to push the synchronized back (with the explanatory comment kept)
if that is what you both prefer. What is the right call here?
Please review as your time permits.
--
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]
