IndexWriterConfig accepts a null event listener, and a later commit() throws NullPointerException

IndexWriterConfig rejects a null it cannot use: setMergePolicy(null), setMergeScheduler(null), setCodec(null), setInfoStream(null) and the other setters listed below throw IllegalArgumentException at once, the class's policy since 4.3.0 (LUCENE-4833). setIndexWriterEventListener(null), added in 9.0.0, has no such check and stores the null. IndexWriter reads the listener once and calls it without a guard, relying on the non-null default IndexWriterEventListener.NO_OP_LISTENER ("A no-op listener that helps to save null checks"). The call happens only when a commit (commit(), prepareCommit() or the final commit of close()) runs a merge-on-commit merge, which is on by default since 9.3.0. So with an otherwise default config the first commit that finds such a merge throws NullPointerException ... because "this.eventListener" is null, far from the setter call. In the tested default setup nothing is lost: that commit writes no new commit point, the writer stays open, and a retried commit(), or close() instead of the retry, commits every document.

1. The setter stores null. IndexWriterConfig.java:572-577: javadoc "Set event listener to record key events in IndexWriter", body this.eventListener = eventListener;, no check. This is the fix site.
2. Everything else assumes non-null. The default is NO_OP_LISTENER (LiveIndexWriterConfig.java:149), whose javadoc says it exists "to save null checks" (IndexWriterEventListener.java:25); the interface is @lucene.experimental (:22).
3. The writer caches it once from the config (IndexWriter.java:951, final field :406).
4. Only a commit's merge-on-commit step calls it. commit() (:4214) leads to prepareCommitInternal (:4252). It reads maxCommitMergeWaitMillis from the config (:3799; default 500, IndexWriterConfig.java:108); when that is above 0 it asks the merge policy for full-flush merges (:3858-3864). If one is found, the writer calls eventListener.beginMergeOnFullFlush (:3890, the NPE) and later endMergeOnFullFlush (:3898), both unguarded. These are the only two call sites. prepareCommit() (:3737) and the final commit of close() (:1365, then shutdown() at :1327) take the same path; getReader also runs full-flush merges with the same wait (:535-694) but never calls the listener.
5. When a merge is found. The default TieredMergePolicy returns one once there are enough small segments: in the repro all 13 one-document segments are merged as one. On a fresh index with one document per commit, commits 1 to 8 succeed and commit 9 throws. With the wait set to 0, or a policy that returns no full-flush merge, the null is never touched.
6. Outcome. The NPE comes before the new commit point is written (startCommit, :3914), so that commit writes no new commit point: the 13th document is flushed into a new segment, but the last commit still holds 12 documents, and the writer is not closed. In this single-threaded default setup it is recoverable: a retried commit() commits all 13 documents, and so does close() with the default commitOnClose, which flushes and waits for merges before it commits (:1312-1338), both right after the failed commit (the fact check, 5 of 5 with assertions on and off) and in place of it (the verifiers). Two limits, not claimed as impact: with a merge policy that still returns a full-flush merge when close() commits, close() throws the same NPE and the writer rolls back (executed); and the failed commit keeps references on its files, a separate commit-bookkeeping defect for any failure before startCommit (LFV-456).

• Who. Code that passes null to setIndexWriterEventListener, for example a framework that forwards an optional listener that was never set. A GitHub code search found no caller of the setter in Elasticsearch, OpenSearch or Solr, and no public report of it was found since 9.0.0 (2021), so the likelihood is rare.
• Default vs opt-in. The null argument is the only non-default; merge-on-commit (500 ms wait) and TieredMergePolicy are the defaults since 9.3.0. In 9.0.0 to 9.2.0 the wait defaulted to 0, so only a writer that switched merge-on-commit on reaches it.
• Effect. A misleading NullPointerException from a commit, at a moment that depends on how many small segments exist, instead of an IllegalArgumentException at the setter. Because early commits on a small index pass, the mistake can survive an application's own tests.
• Not data loss. In the executed single-threaded default setup the throwing commit writes no new commit point and the writer stays open; a retried commit(), or close() instead of the retry, commits every document.
• Scale. 12 one-document segments plus 1 document; one test thread; it failed on every recorded seed.

TestNullIndexWriterEventListener (one method, extends LuceneTestCase, package org.apache.lucene.index of the lucene/core tests, Apache license header). It is the hunt's minimal test plus a guard: an IllegalArgumentException from the setter counts as a pass, so the same test fits either fix. It passes checkGoogleJavaFormat. The test method draws no random values (it uses ByteBuffersDirectory and new IndexWriterConfig(), not the randomized helpers), though the test framework still picks locale, time zone, default codec and similarity from the seed; it was run with seed DEADBEEF, and the hunt's version failed 5 of 5 with --iters 5 and with other seeds. It fails the same way with assertions on and off.

cp $REPRO/TestNullIndexWriterEventListener.java $LUCENE/lucene/core/src/test/org/apache/lucene/index/
cd $LUCENE && ./gradlew :lucene:core:test --tests org.apache.lucene.index.TestNullIndexWriterEventListener -Ptests.seed=DEADBEEF

# gradle, scratch worktree at 5adff78bb168, -Ptests.seed=DEADBEEF (assertions on; -Ptests.asserts=false gives the same)
TestNullIndexWriterEventListener > testCommitWithNullEventListener FAILED
    java.lang.NullPointerException: Cannot invoke "org.apache.lucene.index.IndexWriterEventListener.beginMergeOnFullFlush(org.apache.lucene.index.MergePolicy$MergeSpecification)" because "this.eventListener" is null
        at __randomizedtesting.SeedInfo.seed([DEADBEEF:8DAD77C0050D014D]:0)
        at org.apache.lucene.index.IndexWriter.prepareCommitInternal(IndexWriter.java:3890)
        at org.apache.lucene.index.IndexWriter.commitInternal(IndexWriter.java:4252)
        at org.apache.lucene.index.IndexWriter.commit(IndexWriter.java:4214)
        at org.apache.lucene.index.TestNullIndexWriterEventListener.testCommitWithNullEventListener(TestNullIndexWriterEventListener.java:45)
1 test completed, 1 failed

# with fix A or fix B: 1 test, 0 failures (assertions off); the repro plus 3 suites: 44 tests, 0 failures, 5 skipped

• The class's recorded null policy. lucene/CHANGES.txt:13958-13961 (4.3.0, LUCENE-4833, 9dbfa22b82d, Adrien Grand): "IndexWriterConfig setters now throw an exception when passed null if null is not a valid value."
• The sibling setters follow it with IllegalArgumentException: setOpenMode (IndexWriterConfig.java:160-162), setIndexDeletionPolicy (:221-223), setSimilarity (:258-260), setMergeScheduler (:279-281), setCodec (:297-299), setFlushPolicy (:367, package-private), setInfoStream (:430-436, "To disable logging use InfoStream.NO_OUTPUT"), setInfoStream(PrintStream) (:441-443) and setMergePolicy (LiveIndexWriterConfig.java:258-259). Not every setter is uniform: setIndexSort(null) also fails at the setter, but with a NullPointerException (IndexWriterConfig.java:505-506); and setters whose null is a valid value accept it: documented for setIndexCommit (:235), setSoftDeletesField (:561), setParentField (optional, :580) and setLeafSorter (its getter, LiveIndexWriterConfig.java:423), and for setMergedSegmentWarmer the default that IndexWriter checks for (LiveIndexWriterConfig.java:131, IndexWriter.java:5785).
• The reason behind the policy, as Robert Muir put it on LUCENE-4833 (#5898): "it seems more likely that someone would pass null due to a program mistake, rather than wanting the defaults"; and Adrien Grand: "if someone wants to use the default value, all he has to do is to never call the setter". The policy was kept when it broke a caller (LUCENE-5223, #6287, 2013: the caller was fixed and setInfoStream's check stayed).
• Tests. TestIndexWriterConfig.testInvalidValues (lucene/core/src/test/org/apache/lucene/index/TestIndexWriterConfig.java:196-274) checks the null rejection of the deletion policy, merge scheduler, similarity and merge policy; nothing tests the listener setter with null.
• History. The setter came with LUCENE-9406 (6ba9fe5be35, lucene-solr#2342, issue #10446, 2021) together with the sentinel and the unguarded calls; none of the comments on #10446 or #2342 discusses null.

The strongest counter-argument (the cross-model refute review, verdict not real, high): "Null is not a documented listener value. The experimental listener API supplies NO_OP_LISTENER specifically for callers that want no behavior ... adding an eager null check would improve diagnostics but would not demonstrate incorrect behavior for supported inputs."

Why it does not settle it: the writer cannot run with a null listener (it dereferences it, and NO_OP_LISTENER exists to save null checks), so null is not a valid value here, and the class's recorded policy (LUCENE-4833) is to reject such a null at the setter, as the siblings listed above do; this setter does not. The same reviewer called a fail-fast check "reasonable hardening and consistency work". So it is a consistency and diagnostics fix, not a broken documented contract: the setter's javadoc says nothing about null. The argument does hold for impact: the input is a caller mistake on an experimental API and the failure is loud and recoverable, hence priority Low.

Fixes A and B were built and checked through gradle on 5adff78bb168 in a scratch worktree: the repro with assertions off, then the repro with TestIndexWriterConfig, TestIndexWriterMergePolicy (the merge-on-commit and listener tests) and TestIndexWriterCommit, seed DEADBEEF. The call-site guard option was not run. The regression test for A or B is the one repro test, unchanged.

In IndexWriterConfig.setIndexWriterEventListener (IndexWriterConfig.java:572-577) throw IllegalArgumentException for null and point to the no-op sentinel, as setInfoStream does with InfoStream.NO_OUTPUT (+9/-1 including the javadoc).

  /**
   * Set event listener to record key events in IndexWriter. Must not be null, but {@link
   * IndexWriterEventListener#NO_OP_LISTENER} may be used to record nothing.
   */
  public IndexWriterConfig setIndexWriterEventListener(
      final IndexWriterEventListener eventListener) {
    if (eventListener == null) {
      throw new IllegalArgumentException(
          "eventListener must not be null. "
              + "To record no events use IndexWriterEventListener.NO_OP_LISTENER");
    }
    this.eventListener = eventListener;
    return this;
  }

Trade-offs

Follows the LUCENE-4833 policy as setInfoStream does (an IllegalArgumentException that points to the no-op sentinel), and reports the mistake at the line that made it. Code that passes null today and never reaches a merge-on-commit (wait 0, or few segments) starts failing at configuration time: a behaviour change, but on an experimental API and for an input the class's policy calls invalid.

What the checks validated

The repro passes with assertions off (the setter now throws and the test's guard returns); the repro plus the 3 suites: 44 tests, 0 failures, 5 skipped; :lucene:core:checkGoogleJavaFormat passes with the fix and the test in place.

Store the sentinel when the argument is null (+2/-1).

    this.eventListener =
        eventListener == null ? IndexWriterEventListener.NO_OP_LISTENER : eventListener;

Trade-offs

No new exception, fully compatible. But LUCENE-4833 explicitly turned away null-means-default for this class (Adrien Grand: "all he has to do is to never call the setter"), and it hides a caller's mistake.

What the checks validated

The repro passes with assertions off (commit() now succeeds); the repro plus the 3 suites: 44 tests, 0 failures, 5 skipped. A separate re-run (the verifier's test) and the minimal repro's recorded runs also pass with this change shadowed in.

Check eventListener != null before beginMergeOnFullFlush and endMergeOnFullFlush (IndexWriter.java:3890, :3898).

Trade-offs

Spreads the null checks the sentinel exists to avoid, and getIndexWriterEventListener() would still return null to other callers.

What the checks validated

Not run.

Method: semi-formal (a proof-obligation audit of a class's contract, witnessed by an executable repro).

• The finder listed what IndexWriter assumes about its configuration: the cached listener is called unguarded at IndexWriter.java:3890 and :3898, so every value IndexWriterConfig can hold must be non-null. The sibling setters discharge this for their own fields with a null check (the LUCENE-4833 policy); setIndexWriterEventListener does not.
• The finder then wrote a public-API JUnit test that fails on main with assertions on and off, and on 10.6.0 and 9.12.3; an independent reproducer turned it into the default-config minimal test.
• Two executed cross-model reviews, a devil's-advocate verifier and an adjudication reproduced it (the adjudication with the reproducer's test, the others with their own) ("Prior reports, history and checks").