visit2rahul commented on PR #4487: URL: https://github.com/apache/polaris/pull/4487#issuecomment-4492879373
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 8a4c14401: - 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. -- 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]
