[#1026] Close the import config on every path an import task can end on

[vharseko]

Fixes #1026.

The leak

ImportTask closed its LDIFImportConfig after the outer try/finally, so only a fully successful import reached it: a backend that could not be disabled, a lock that could not be taken, an importLDIF() that threw, a lock that could not be released and a backend that could not be re-enabled all returned above it.

What that actually leaves open is narrower than the config as a whole. Every backend closes the config together with its LDIF reader (LDIFReader.close() → importConfig.close(), inside the backends' try-with-resources), so an import that got as far as constructing the reader is closed - and its rejects file flushed - whichever way it ends. What stayed open were the reject and skip files of an import that failed before that reader existed: the task opens them before the backend is touched, and a failed disable, a failed lock, or an importLDIF() that dies opening the LDIF (a missing file, a directory, a backend still online) never reached a reader. They stayed open for as long as the completed task is retained.

The change

importConfig.close() now opens the finally, ahead of the re-enable and the listener notification, so every return below it reaches the close. No try of its own: StaticUtils.close() swallows the IOException, and closing a config the backend already closed is a no-op.

One return is above that finally and is closed where it stands. The reject file is opened before the skip file, and the skip file is opened on a path the task never checks - initializeTask only stores the attribute as a string - so an import whose skip file cannot be opened returns STOPPED_BY_ERROR with the reject writer already open, above notifyImportBeginning and above the try. That catch closes the config itself.

The two files are not moved inside the try instead, which would have needed no second close: a listener told an import began puts back what it took offline when it is told it ended - a replication domain reloads and rewinds its state - and an import which never reached a backend has nothing for a listener to put back. The mirror return, the reject file itself failing to open, leaks nothing close() would close: the LDIF reader is lazy, so at that point nothing is open.

Tests

In TestImportAndExport, three roads, each measured against a mutant of ImportTask:

• testFailedImportClosesItsRejectAndSkipFiles - an import whose LDIF path is a directory, accepted by the task validation and failing inside importLDIF() before the backend creates its reader, with a reject and a skip file set. After the task ends, a write to either writer must be refused as Stream closed.
• testImportEndsWhenTheBackendCannotBeDisabled now carries a reject and a skip file too. It is the road which ends furthest from the backend, so a close which rides on the backend having been disabled, or on the import having been launched, passes the first case and fails this one.
• testImportWhichCannotOpenItsSkipFileClosesItsRejectFile - a valid LDIF with a directory as the skip file. That return is above the notification the other two read the config through, so the config is read from the task the scheduler kept, and the case also asserts the skip writer was never assigned.

TestTaskListener keeps the config it was handed at processImportEnd for the first two.

Each of the three mutants below is one failure out of sixteen, on exactly one case, with the rest of the class green:

mutant	red case
the close in the skip catch dropped	testImportWhichCannotOpenItsSkipFileClosesItsRejectFile
the close moved into the lock-release finally under if (backendDisabled)	testImportEndsWhenTheBackendCannotBeDisabled
the close back after the outer try/finally, plus one in catch (DirectoryException)	testImportEndsWhenTheBackendCannotBeDisabled

Runs

TestImportAndExport 16/16, LDIFBackendTestCase 22/22, PrivilegeTestCase 185/185, ReSyncTest 2/2.

Out of scope, noticed on the way

• With ds-task-import-overwrite-rejects: TRUE the reject file is opened - and truncated - before the backend is disabled, so an import that fails on the disable has already emptied the previous run's rejects. Pre-existing, unrelated to the close.
• A template import starts its generator thread when the config is built (new LDIFImportConfig(tf) → MakeLDIFInputStream.newStartedInputStream()), and LDIFImportConfig.close() closes only the reader and the two writers - never the input stream it built for itself. On every road fixed here the reader is null, because getReader() was never called, so a failed template import still leaves a live, non-daemon MakeLDIF Input Stream Thread behind. That is inside LDIFImportConfig.close() rather than in this task: A MakeLDIF import which fails before its reader leaves the generator thread running #1078.

[vharseko]

@maximthomas rebased onto master after #969 merged (e86f702f8e): the stacked copy of that PR's commit is dropped, so the branch is now the single commit 5b4444f sitting directly on master.

Nothing in the change itself moved - git range-diff shows the patch byte-identical to d23c23d, and the three files it touches (ImportTask, TestTaskListener, TestImportAndExport) are the same on master as on the base the PR was tested against, so the runs listed in the description still stand. The description no longer mentions the stack.

[maximthomas]

praise: The close now sits where every road of runTask unwinds, and the test really pins it.

• importConfig.close() is the first statement of the outer finally (ImportTask.java:726), ahead of the re-enable and notifyImportEnded, so the disable-fail (:650), lock-fail (:661, :669) and importLDIF() (:678, :692) returns all reach it; no try of its own is right — StaticUtils.close swallows the IOException, and a second close of the reader-owned config is idempotent on every stream getReader() stacks.
• testFailedImportClosesItsRejectAndSkipFiles is measured red with ImportTask.java at the base and the test at the head — 15 ran, 1 failed, The reject writer is still open after the import ended — and green on every ubuntu cell at the head.
• TestTaskListener.lastImportEndConfig reads the config through the listener road the server already offers instead of reaching into the task.

---

issue (blocking): The skip-file open failure still returns with the reject writer open — above the finally the close was moved into.

opendj-server-legacy/src/main/java/org/opends/server/tasks/ImportTask.java:617-621, :600, :615

writeRejectedEntries at :600 opens the reject file (LDIFImportConfig.newBufferedWriter:373-375, a FileWriter — truncated under ds-task-import-overwrite-rejects: TRUE). When writeSkippedEntries at :615 then fails — an unwritable path, a directory; initializeTask only stores the attribute as a string — the catch at :617 returns STOPPED_BY_ERROR at :620, above notifyImportBeginning (:628) and above the try at :636, so the close at :726 never runs and nothing else closes the config (interruptTask only calls cancel(); no finalizer, no task-level cleanup). The block is unchanged by this PR, but it is the one early return left that leaks what the title says is closed on every path, and its mirror (reject open failing at :604) leaks nothing because nothing is open yet.

      catch (Exception e)
      {
        logger.error(ERR_LDIFIMPORT_CANNOT_OPEN_SKIP_FILE, skipFile, getExceptionMessage(e));
        // The reject file is already open; no finally below this return closes it.
        importConfig.close();
        return TaskState.STOPPED_BY_ERROR;
      }

Pin: the new test's observable cannot see this road — the return happens before notifyImportBeginning, so processImportEnd never fires — a pin needs another handle on the config; the one-line close is the required part.

---

suggestion (non-blocking): Only the importLDIF()-throws road is pinned; the disable-fail and lock-fail roads the description names are closed by construction.

opendj-server-legacy/src/test/java/org/opends/server/tasks/TestImportAndExport.java:461-494, :397-425

The directory-as-LDIF injection fails inside backend.importLDIF (ImportTask.java:675), i.e. inside the innermost try (:673) with backendDisabled == true. A close moved into the lock-release finally (:699-700), guarded with if (backendDisabled), or put back at its old place with an extra importConfig.close() in the catch (DirectoryException) arm at :678, still closes the writers on that road and stays green across the class, while the disable-fail return at :650 leaks again. testImportEndsWhenTheBackendCannotBeDisabled already has the fixture; it sets no reject or skip file and asserts only the begin/end counts.

      // in testImportEndsWhenTheBackendCannotBeDisabled, with a temp skip file as in the new case
      Entry taskEntry = TestCaseUtils.makeEntry(
          "dn: ds-task-id=" + UUID.randomUUID() + ",cn=Scheduled Tasks,cn=Tasks",
          "objectclass: top",
          "objectclass: ds-task",
          "objectclass: ds-task-import",
          "ds-task-class-name: org.opends.server.tasks.ImportTask",
          "ds-task-import-backend-id: " + backendID,
          "ds-task-import-ldif-file: " + ldifFile.getPath(),
          "ds-task-import-reject-file: " + rejectFile.getPath(),
          "ds-task-import-skip-file: " + skipFile.getPath(),
          "ds-task-import-overwrite-rejects: TRUE");

      TestTaskListener.lastImportEndConfig.set(null);
      testTask(taskEntry, TaskState.STOPPED_BY_ERROR, 60);

      assertEquals(TestTaskListener.importBeginCount.get(), importBeginCount + 1);
      assertEquals(TestTaskListener.importEndCount.get(), importEndCount + 1);
      LDIFImportConfig importConfig = TestTaskListener.lastImportEndConfig.get();
      assertNotNull(importConfig, "The import end was not notified");
      assertClosed(importConfig.getRejectWriter(), "reject");
      assertClosed(importConfig.getSkipWriter(), "skip");

Pin: the writers open at :600/:615 before the disable at :640, and notifyImportEnded (:749) runs on that road, so under any of the three mutants the disable-fail road hands open writers to the listener and assertClosed goes red. The lock-fail road has no injection in the tree and stays by construction.

[vharseko]

@maximthomas both taken, round 2 is e4921bcac7.

issue (blocking) - the skip-file open failure leaks the reject writer

Confirmed and fixed. writeRejectedEntries at :600 opens the file eagerly (LDIFImportConfig.newBufferedWriter:367-386 builds the FileWriter the moment it is called), writeSkippedEntries at :615 can throw on a path the task never checks (initializeTask:203 only stores the attribute as a string), and the catch at :617 returns above both notifyImportBeginning (:628) and the try at :636. Nothing else closes it: the only other uses of the field are the cancel() in interruptTask and the close() this PR moved.

importConfig.close() now opens that catch arm, as you wrote it.

I did not take the other road out of it - opening the two files below notifyImportBeginning, inside the try, so that the existing finally closes them and no new close is needed. It looks like the structural fix, but the listeners pay for it: MultimasterReplication.processImportBegin/End (:664, :676) is domain.disable() / domain.enable(), and the price of an enable() is a replication domain reloading and rewinding its state - which is what testFailedImportEndsOnlyOnce exists to hold to one. An import which could not open its own files never touched a backend and has nothing for a listener to put back, so it should keep costing nothing. The comment in the catch says that, so the next reader does not "simplify" it away.

The mirror return at :605 stays as it is: you are right that nothing close() closes is open there - getReader() is lazy (LDIFImportConfig:223-252), reader is still null, and a close would be dead code. What is alive there for a template import is not reachable by close() at all; last section.

Pin. testImportWhichCannotOpenItsSkipFileClosesItsRejectFile: a valid LDIF with a directory as the skip file. As you said, processImportEnd never fires on that road, so the config is read from the task the scheduler kept - TasksTestCase.getDoneTask() already hands out the Task, and the field is reached by reflection rather than by widening ImportTask's surface for a test. The case also asserts the skip writer was never assigned, so it fails loudly rather than passing for the wrong reason if that path ever became writable.

suggestion (non-blocking) - only the importLDIF road was pinned

Taken as written. testImportEndsWhenTheBackendCannotBeDisabled now sets a reject file and a temp skip file and asserts both writers closed through lastImportEndConfig, and its javadoc says why that road is the one worth holding: it ends furthest from the backend, so a close which rides on the backend having been disabled, or on the import having been launched, misses it.

Measured, TestImportAndExport at the head of round 2 against three mutants of ImportTask - each is one failure out of sixteen, on exactly one case, with the rest of the class green:

mutant	red case
the new importConfig.close() dropped from the skip catch	testImportWhichCannotOpenItsSkipFileClosesItsRejectFile - The reject writer is still open after the import ended
the close moved into the lock-release finally (:699) under if (backendDisabled)	testImportEndsWhenTheBackendCannotBeDisabled - same assert
the close back after the outer try/finally, plus one in catch (DirectoryException) (:678)	testImportEndsWhenTheBackendCannotBeDisabled - same assert

The lock-fail road still has no injection in the tree and stays by construction.

Noticed while checking the mirror return

A template import - ds-task-import-template-file - starts its generator thread at construction: new LDIFImportConfig(tf) (ImportTask:566) goes to MakeLDIFInputStream.newStartedInputStream(), which calls generatorThread.start(). That thread ends only when MakeLDIFInputStream.close() sets closed, and LDIFImportConfig.close() never reaches it - it closes reader, rejectWriter and skipWriter, and on every one of these roads reader is null because getReader() was never called. So each return this PR fixes still leaves a live, non-daemon MakeLDIF Input Stream Thread spinning against a full queue when the import was template-driven, and import-ldif --templateFile failing that way does not give the shell back.

That is inside LDIFImportConfig.close() rather than in this task, so it is not folded in here: #1078.

Runs

Round 2, head of the branch:

• TestImportAndExport 16/16 (fifteen before, one new case)
• LDIFBackendTestCase 22/22
• ReSyncTest 2/2
• PrivilegeTestCase 185/185

PrivilegeTestCase is the run on its own. In the combined run its setUp lost 0.0.0.0:65532 to the class before it (Address already in use), which is the local port collision between consecutive server classes, not this change.