nandorKollar commented on PR #4487: URL: https://github.com/apache/polaris/pull/4487#issuecomment-4496953151
> Thank you @nandorKollar and @adutra for the careful review on this. > > @nandorKollar - you are right that UnicastProcessor.onNext is already declared `public synchronized void` in smallrye-mutiny, so the external lock wrapper was acquiring the same monitor twice. The original premise of this PR (caller-enforced sequential onNext) was based on the Reactive Streams contract; I should have read the smallrye source before proposing the synchronization. Thank you for the catch. > > Re-scoped the PR in [8a4c144](https://github.com/apache/polaris/commit/8a4c14401598c62bfbe864d21a0bc9a9be4f5252): > > * Removed the redundant synchronized(processor) block > * Added a comment above the onNext() call explaining why concurrent calls are safe (per @adutra's suggestion) > * Dropped testProcessEventIsThreadSafe since it tested smallrye-mutiny's internal synchronization rather than Polaris code, and was non-deterministic as @nandorKollar noted > > The PR is now a small documentation contribution clarifying the existing thread-safety contract. > > For the separate onComplete() race that @nandorKollar flagged - agreed with @adutra that it belongs in its own PR. I will file a follow-up issue and take the work, so it has a clear owner. > > Please review as your time permits. Thanks for adjusting the PR accordingly. Actually the extra synchronized block doesn't cause any harm (maybe a minor performance degradation, if any at all), but looks unnecessary at least now. However, I can see one possible benefit keeping it: in case in the future, synchronized from `UnicastProcessor.onNext` is removed, then the extra locking in this PR makes sense. As the contract of `UnicastProcessor` doesn't state, that it is safe to use in multi-threaded environment, it might change in the future. @visit2rahul @adutra what do you think? -- 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]
