Skip to content

Clear the database appender buffer when committing fails - #4319

Open
dajiaohuang wants to merge 1 commit into
apache:2.xfrom
dajiaohuang:fix/4318-jdbc-buffer-flush-leak
Open

dajiaohuang wants to merge 1 commit into
apache:2.xfrom
dajiaohuang:fix/4318-jdbc-buffer-flush-leak

Conversation

@dajiaohuang

Copy link
Copy Markdown

Fixes #4318.

AbstractDatabaseManager#flush() handed the buffered events to
writeInternal() and then ran commitAndClose() and buffer.clear() in
the same finally block:

} finally {
    this.commitAndClose();
    // not sure if this should be done when writing the events failed
    this.buffer.clear();
}

When commitAndClose() throws (a database failure during the commit/close
phase), the clear() was skipped. The events therefore stayed in the
buffer and were written a second time by the following flush, and, because
the buffer keeps accumulating new events while the failure persists, it
grew without bound. flush() is also called from shutdown(), so a
failing commit at shutdown could also mask the original exception with a
secondary one from re-sending the same events.

The fix moves buffer.clear() into an inner finally block, so the events
are discarded no matter how commitAndClose() returns or throws. The
events were already passed to the database layer, so keeping them cannot
recover the transaction.

testBufferedEventsAreDiscardedWhenCommitFails makes commitAndClose()
fail once and then succeed, and asserts that writeInternal() is invoked
exactly twice in total (once per event), i.e. that the failed transaction
is not re-sent.

Note on scope: connectAndStart() is deliberately left outside the try,
so events are still retried when the connection cannot be established.
Only the commit/close phase changes behaviour here.

Checklist

  • Base your changes on 2.x branch if you are targeting Log4j 2; use main otherwise
  • ./mvnw verify succeeds (the build instructions)
  • Non-trivial changes contain an entry file in the src/changelog/.2.x.x directory
  • Tests are provided

`AbstractDatabaseManager#flush` cleared the buffer in a `finally` block
that also contained `commitAndClose`. If committing or closing the
transaction threw, the `clear()` was skipped, so the events stayed in the
buffer and were sent to the database a second time by the next flush.
Since `flush()` is also called from `shutdown()`, and the buffer only
grows while the failure persists, this leaked log events and produced
duplicate rows.

Move `buffer.clear()` into an inner `finally` block so that events are
discarded regardless of the outcome of `commitAndClose`.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Development

Successfully merging this pull request may close these issues.

Memory leak in JdbcAppender when a database failure occurs during the commit/close phase.

1 participant