mattisonchao opened a new pull request, #15837: URL: https://github.com/apache/pulsar/pull/15837
### Motivation In current implementation, We have more than one potential NPE relate `ManagedLedgerImpl#startReadOperationOnLedger` method. **1. Unbox NPE** https://github.com/apache/pulsar/blob/b13d15c4d6d795f33c6ed23f920fabbb3eeaf745/managed-ledger/src/main/java/org/apache/bookkeeper/mledger/impl/ManagedLedgerImpl.java#L2230-L2244 According to this code, we can know that when the ledger id is null, we will fail `OpReadEntry`. However, there is no direct return here after failure. NPE will be thrown on line 2238. Because we want to unbox `null`. **2. null `OpReadEntry` context** According to the code above, we can see when we fail `OpReadEntry`, we pass a null value as context(line 2235). there is an NPE when the dispatcher gets the callback. https://github.com/apache/pulsar/blob/0975cdc2489739b7518ee7acc78bbc6e8c8e2e4d/pulsar-broker/src/main/java/org/apache/pulsar/broker/service/persistent/PersistentDispatcherSingleActiveConsumer.java#L478-L484 **3. `OpReadEntry#create`** https://github.com/apache/pulsar/blob/93761284b9f6875da0403f5fedb6ccbfbbcd7315/managed-ledger/src/main/java/org/apache/bookkeeper/mledger/impl/OpReadEntry.java#L48-L63 When we create `OpReadEntry` and then call `ManagedCursor#startReadOperationOnLedger` , assuming the ledger id is equal to null. At this point, we will fail this `OpReadEntry`. But at the current time, other parameters are not initialized. When we call `OpReadEntry#readEntriesFailed`. The cursor will be null and the NPE will be thrown. https://github.com/apache/pulsar/blob/93761284b9f6875da0403f5fedb6ccbfbbcd7315/managed-ledger/src/main/java/org/apache/bookkeeper/mledger/impl/OpReadEntry.java#L90-L94 ### Modifications - Remove fail `OpReadEntry` logic in `ManagedCursor#startReadOperationOnLedger`, when ledger id equals null, we can return the original value. the `ManagedLedger` will validate if this operation is legal. From the perspective of the overall design, the `OpReadEntry` is just a middle state object, that may have illegal value, we have to check this `OpReadEntry` is valid in `ManagedLedger#asyncReadEntries`(Current we already do that). ### Verifying this change - [x] Make sure that the change passes the CI checks. ### Documentation - [x] `doc-not-needed` (Please explain why) -- 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]
