Aias00 commented on code in PR #7141:
URL: https://github.com/apache/shenyu/pull/7141#discussion_r4060323559
##########
shenyu-common/src/main/java/org/apache/shenyu/common/concurrent/MemorySafeLinkedBlockingQueue.java:
##########
@@ -30,9 +30,9 @@ public class MemorySafeLinkedBlockingQueue<E> extends
LinkedBlockingQueue<E> {
private static final long serialVersionUID = 8032578371749960142L;
- private int maxFreeMemory;
+ private volatile int maxFreeMemory;
Review Comment:
`volatile` is the right fix and it is genuinely needed here - unlike a plain
immutable field, both values have public setters (`setMaxFreeMemory` /
`setRejector`), so they really are mutable shared state read from `put`/`offer`
on arbitrary threads.
Two notes, neither blocking:
1. `hasRemainedMemory()` is still check-then-act:
```java
if (!hasRemainedMemory()) { rejector.reject(e, this); return false; }
return super.offer(e);
```
`volatile` gives visibility for `maxFreeMemory`/`rejector`, it does not make
the memory check atomic with the enqueue. That is fine for a best-effort OOM
guard - arguably publishing the config safely is exactly what was missing - but
worth a comment so nobody reads `volatile` as "the guard is atomic".
2. Same as elsewhere in this batch: asserting `Modifier.isVolatile(...)` by
reflection checks an implementation detail, and would silently pass even if the
setter were removed and the field became effectively final. A behavioural test
(e.g. `setMaxFreeMemory(0)` from one thread while another offers) is more
valuable, though harder to make deterministic - so keeping this as a cheap
regression guard is acceptable.
--
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]
