eolivelli opened a new pull request, #4760:
URL: https://github.com/apache/bookkeeper/pull/4760
## Motivation
`monitorPendingAddOps()` iterates over the `pendingAddOps` queue and calls
`maybeTimeout()` on each op. Concurrently, `sendAddSuccessCallbacks()` can
remove a completed op from the queue, call `submitCallback()` on it, and
ultimately trigger `recyclePendAddOpObject()` which sets `clientCtx = null`.
If the scheduler thread still holds a live iterator reference to the same op
and then calls `maybeTimeout()`, dereferencing `clientCtx` causes a
`NullPointerException`:
```
java.lang.NullPointerException
at
org.apache.bookkeeper.client.PendingAddOp.maybeTimeout(PendingAddOp.java:157)
at
org.apache.bookkeeper.client.LedgerHandle.monitorPendingAddOps(LedgerHandle.java:2063)
```
The race is only triggered when `addEntryQuorumTimeoutNanos > 0` (i.e. the
quorum-timeout monitor is actually scheduled). It became reliably observable
with **Netty 4.1.130**, which changed `Recycler` thread-scheduling behavior
and narrowed the window enough to expose the pre-existing race.
Closes #4759.
## Changes
### `PendingAddOp.java`
* **`clientCtx` → `volatile ClientContext clientCtx`**
`recyclePendAddOpObject()` is `synchronized` but `maybeTimeout()` is not.
Without `volatile` the JMM does not guarantee that the `null` write in
`recyclePendAddOpObject()` is visible to the unsynchronized read in
`maybeTimeout()`. Making the field `volatile` (consistent with the existing
`volatile long requestTimeNanos`) closes the visibility gap.
* **Null guard at the top of `maybeTimeout()`**
If `clientCtx` is `null` the op has already been recycled and its
add-entry completed; there is nothing to time out, so the method
returns `false` immediately.
### `PendingAddOpTest.java`
Three new unit tests:
| Test | What it verifies |
|---|---|
| `testMaybeTimeoutReturnsFalseWhenClientCtxIsNull` | The exact race:
`clientCtx = null` must not NPE |
| `testMaybeTimeoutReturnsFalseWhenWithinQuorumTimeout` | Normal path — not
yet timed out |
| `testMaybeTimeoutReturnsTrueWhenQuorumTimeoutExpired` | Normal path —
timed out, `true` returned |
## Testing
```
mvn -pl bookkeeper-server -Dtest='PendingAddOpTest' test
```
```
Tests run: 4, Failures: 0, Errors: 0, Skipped: 0
BUILD SUCCESS
```
🤖 Implemented by the `pr-worker` agent.
--
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]
