lymerin commented on PR #7495: URL: https://github.com/apache/shenyu/pull/7495#issuecomment-6079965354
> Hi, @lymerin, thanks for your PR. I do not see a blocking issue here. > > #### Verification > On `c60c404a8`: > > `./mvnw -pl shenyu-common test -Dtest='MemoryLimitedLinkedBlockingQueueTest,MemoryLimiterTest'` > > * 24 tests, 0 failures, including the two new wake-up tests. > * `checkstyle:check` passes for `shenyu-common`. > > #### A few notes > * `volatile memoryLimit` plus the write under `acquireLock` is the right fix: `acquire(Object)` reads the limit without the lock while the waiting paths read it inside the lock, so visibility is now guaranteed on both sides. > * Writing the limit and signalling while holding `acquireLock` (rather than `fullyLock`) is sufficient: waiters only wait on `notLimited` under `acquireLock`, and they re-check the predicate in the loop, so an over-signal costs nothing. > * `reset()` is reached from `MemoryLimitedLinkedBlockingQueue.clear()` and takes `fullyLock` itself, so no acquire/release can run concurrently and `LongAdder.reset()` is safe here. A short comment would be worth adding, because `LongAdder.reset()` is only correct without concurrent updates and the old `sumThenReset()` hid that requirement. > * The new tests are deterministic (mocked `Instrumentation.getObjectSize` sizes plus a wait-queue-length probe) and assert the interesting asymmetry: the small producer is woken after the limit is raised while the large one stays blocked. Both the limit-increase and the clear-after-shrink paths are covered. > > #### Possible Improvements > * No material follow-up. If you ever want to signal only the producers that can now make progress, comparing with the previous limit before signalling would do it, but signalling all waiters is simpler and correct given the loop re-check. Thanks for the review. I agree with keeping signalAll() here. I also checked the usage path: setMemoryLimit() is an explicit limit-update operation, and reset() is reached from MemoryLimitedLinkedBlockingQueue.clear(), so neither is on the normal high-frequency queue admission/release path. Given that, I don't think selectively signalling waiters would provide a meaningful performance benefit here. Waking all waiters and letting each one re-check the predicate keeps the behavior simple and correct, while avoiding additional state or selection logic. I’ll keep the current signalAll() approach and add a short comment around LongAdder.reset() to document that fullyLock() prevents concurrent acquire/release updates during the reset. -- 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]
