[managedledger] NPE on OpAddEntry while ManagedLedger is closing

[nicoloboschi]

Motivation

The test ManagedLedgerBkTest#managedLedgerClosed closes ManagedLedger object between some asyncAddEntry operations and sometimes it fails with this error

2021-10-14T10:21:58,294+0200 ERROR [BookKeeperClientWorker-OrderedExecutor-0-0] o.a.b.c.u.SafeRunnable@38 - Unexpected throwable caught  {}
java.lang.NullPointerException: Cannot invoke "org.apache.bookkeeper.client.LedgerHandle.getId()" because "this.ledger" is null
	at org.apache.bookkeeper.mledger.impl.OpAddEntry.addComplete(OpAddEntry.java:155) ~[classes/:?]
	at org.apache.bookkeeper.client.AsyncCallback$AddCallback.addCompleteWithLatency(AsyncCallback.java:92) ~[bookkeeper-server-4.14.2.jar:4.14.2]
	at org.apache.bookkeeper.client.PendingAddOp.submitCallback(PendingAddOp.java:431) ~[bookkeeper-server-4.14.2.jar:4.14.2]
	at org.apache.bookkeeper.client.LedgerHandle.errorOutPendingAdds(LedgerHandle.java:1799) ~[bookkeeper-server-4.14.2.jar:4.14.2]
	at org.apache.bookkeeper.client.LedgerHandle$5.safeRun(LedgerHandle.java:574) ~[bookkeeper-server-4.14.2.jar:4.14.2]
	at org.apache.bookkeeper.common.util.SafeRunnable.run(SafeRunnable.java:36) [bookkeeper-common-4.14.2.jar:4.14.2]
	at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1130) [?:?]
	at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:630) [?:?]
	at io.netty.util.concurrent.FastThreadLocalRunnable.run(FastThreadLocalRunnable.java:30) [netty-common-4.1.68.Final.jar:4.1.68.Final]
	at java.lang.Thread.run(Thread.java:832) [?:?]

I figured out that during the closing of the ManagedLedger, the pending OpAddEntry objects are set to failed and recycled. During these operations, BK ledger has been closed and it tries to make PendingAddOp objects to call their callbacks as well. So we end up with, for the first entry added after the ManagedLedger closing procedure:

• failed callback is called (AddEntryCallback#addFailed)
• the Java object is recycled ( OpAddEntry#recycle )
• BK calls the callback which expect the object to be "running" (AddCallback#addComplete)

Modifications

When ManagedLedger signals to the OpAddEntry to fail, now the state is updated to CLOSED. In this way the BK callback has no effect, the object is correctly recycled and the "failed" callback is correctly triggered.

After this modification, the ManagedLedgerBkTest#managedLedgerClosed test has never failed locally. All managed-ledgers tests pass locally.

No more tests needed since ManagedLedgerBkTest#managedLedgerClosed already tests this behaviour (I could make more precise test (by using mockito) but I found out that is pretty hard and IMHO not really needed)

Verifying this change

This change is already covered by existing tests, such as ManagedLedgerBkTest.

Does this pull request potentially affect one of the following parts:

If yes was chosen, please highlight the changes

• Dependencies (does it add or upgrade a dependency): (no)
• The public API: (no)
• The schema: (no)
• The default values of configurations: (no)
• The wire protocol: (no)
• The rest endpoints: (no)
• The admin cli options: (no)
• Anything that affects deployment: (no)

Documentation

Check the box below and label this PR (if you have committer privilege).

Need to update docs?
[x] no-need-doc

[nicoloboschi]

/pulsarbot run-failure-checks

[nicoloboschi]

/pulsarbot run-failure-checks

[nicoloboschi]

/pulsarbot run-failure-checks

[nicoloboschi]

/pulsarbot run-failure-checks

[eolivelli]

+1 good catch @nicoloboschi