[
https://issues.apache.org/jira/browse/CASSANDRA-21616?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel
]
Michael Semb Wever updated CASSANDRA-21616:
-------------------------------------------
Test and Documentation Plan:
CommitLogTest, BatchCommitLogTest and GroupCommitLogTest pass together (382
tests, JDK 17), as do CommitlogShutdownTest, CommitLogInitWithExceptionTest and
CommitLogSegmentBackpressureTest.
The patch carries no new test. The leak needs the allocator thread to sit
between the state test in InfiniteLoopExecutor.loop and the synchronized
segment creation block at the moment shutdown runs, and the interrupt handler
synchronizes on the task, so a latch inside createSegment deadlocks against
shutdown instead of reproducing the window. A deterministic test would have to
replace the allocator executor. InfiniteLoopExecutorTest already asserts the
executor contract the patch relies on: shutdown delivers precisely one more
task run, shutdownNow delivers none.
was:
CommitLogTest, BatchCommitLogTest, GroupCommitLogTest and CommitlogShutdownTest
pass on all
three branches: 380 tests, 0 failures, 3 skipped, JDK 17.
The patch carries no new test. The leak needs the allocator thread to sit
between the state
test in InfiniteLoopExecutor.loop and the synchronized segment creation block
at the moment
shutdown runs. The interrupt handler synchronizes on the task, so a latch inside
createSegment deadlocks against shutdown instead of reproducing the window; a
deterministic
test would have to replace the allocator executor. InfiniteLoopExecutorTest
already asserts
the executor contract the patch relies on: shutdown delivers precisely one more
task run,
shutdownNow delivers none.
Description:
{{AbstractCommitLogSegmentManager.shutdown}} calls {{executor.shutdownNow()}},
which puts the {{InfiniteLoopExecutor}} into {{SHUTTING_DOWN_NOW}}.
{{InfiniteLoopExecutor.loop}} tests that state at the top of the loop and
breaks before it runs the task:
{code:java}
Object cur = state;
if (cur == SHUTTING_DOWN_NOW) break;
interrupted |= Thread.interrupted();
if (cur == NORMAL && interrupted) cur = INTERRUPTED;
task.run((State) cur);
{code}
So {{AllocatorRunnable}} never receives {{Interruptible.State.SHUTTING_DOWN}},
and this branch of it is unreachable:
{code:java}
case SHUTTING_DOWN:
// If shutdown() started and finished during segment creation, we are now
left with a
// segment that no one will consume. Discard it.
discardAvailableSegment();
return;
{code}
The branch guards a real interleaving. A shutdown that starts while the
allocator sits between the state test and the segment creation block finds no
available segment to discard. The allocator then clears the interrupt, creates
a segment and assigns it, and the loop breaks on the next pass. Nothing
consumes that segment and nothing discards it, so its file stays in the commit
log directory and its buffer is not released. {{awaitTermination}} only closes
the segments in {{activeSegments}}.
CASSANDRA-16925 introduced the condition when it moved the allocator onto
{{Interruptible}}. The code it replaced held a {{shutdown}} flag that the
allocator tested straight after {{createSegment}}, in the same loop pass, so
the discard ran.
The patch calls {{executor.shutdown()}}, which delivers one final
{{SHUTTING_DOWN}} pass to the task and restores the discard.
Patch:
[mck/upstream/commitlog-allocator-shutdown/5.0|https://github.com/thelastpickle/cassandra/tree/mck/upstream/commitlog-allocator-shutdown/5.0]
Provenance:
[1976b540ca|https://github.com/datastax/cassandra/commit/1976b540ca8e63205f6d8a56aa0c7f4a6b2112e9]
by [~brandon.williams]
was:
{{AbstractCommitLogSegmentManager.shutdown}} calls {{executor.shutdownNow()}},
which puts the {{InfiniteLoopExecutor}} into {{SHUTTING_DOWN_NOW}}.
{{InfiniteLoopExecutor.loop}} tests that state at the top of the loop and
breaks before it runs the task:
{code:java}
Object cur = state;
if (cur == SHUTTING_DOWN_NOW) break;
interrupted |= Thread.interrupted();
if (cur == NORMAL && interrupted) cur = INTERRUPTED;
task.run((State) cur);
{code}
So {{AllocatorRunnable}} never receives {{Interruptible.State.SHUTTING_DOWN}},
and this branch of it is unreachable:
{code:java}
case SHUTTING_DOWN:
// If shutdown() started and finished during segment creation, we are now
left with a
// segment that no one will consume. Discard it.
discardAvailableSegment();
return;
{code}
The branch guards a real interleaving. A shutdown that starts while the
allocator sits between the state test and the segment creation block finds no
available segment to discard. The allocator then clears the interrupt, creates
a segment and assigns it, and the loop breaks on the next pass. Nothing
consumes that segment and nothing discards it, so its file stays in the commit
log directory and its buffer is not released. {{awaitTermination}} only closes
the segments in {{activeSegments}}.
CASSANDRA-16925 introduced the condition when it moved the allocator onto
{{Interruptible}}. The code it replaced held a {{shutdown}} flag that the
allocator tested straight after {{createSegment}}, in the same loop pass, so
the discard ran.
The patch calls {{executor.shutdown()}}, which delivers one final
{{SHUTTING_DOWN}} pass to the task and restores the discard.
Patch:
[mck/CASSANDRA-21616/5.0|https://github.com/thelastpickle/cassandra/tree/mck/CASSANDRA-21616/5.0]
Provenance:
[1976b540ca|https://github.com/datastax/cassandra/commit/1976b540ca8e63205f6d8a56aa0c7f4a6b2112e9]
by Brandon Williams
> The commit log allocator never observes the shutdown state, so a prepared
> segment can leak
> ------------------------------------------------------------------------------------------
>
> Key: CASSANDRA-21616
> URL: https://issues.apache.org/jira/browse/CASSANDRA-21616
> Project: Apache Cassandra
> Issue Type: Bug
> Components: Local/Commit Log
> Reporter: Michael Semb Wever
> Assignee: Michael Semb Wever
> Priority: Normal
> Fix For: 5.0.x, 6.0.x, 7.x
>
>
> {{AbstractCommitLogSegmentManager.shutdown}} calls
> {{executor.shutdownNow()}}, which puts the {{InfiniteLoopExecutor}} into
> {{SHUTTING_DOWN_NOW}}. {{InfiniteLoopExecutor.loop}} tests that state at the
> top of the loop and breaks before it runs the task:
> {code:java}
> Object cur = state;
> if (cur == SHUTTING_DOWN_NOW) break;
> interrupted |= Thread.interrupted();
> if (cur == NORMAL && interrupted) cur = INTERRUPTED;
> task.run((State) cur);
> {code}
> So {{AllocatorRunnable}} never receives
> {{Interruptible.State.SHUTTING_DOWN}}, and this branch of it is unreachable:
> {code:java}
> case SHUTTING_DOWN:
> // If shutdown() started and finished during segment creation, we are now
> left with a
> // segment that no one will consume. Discard it.
> discardAvailableSegment();
> return;
> {code}
> The branch guards a real interleaving. A shutdown that starts while the
> allocator sits between the state test and the segment creation block finds no
> available segment to discard. The allocator then clears the interrupt,
> creates a segment and assigns it, and the loop breaks on the next pass.
> Nothing consumes that segment and nothing discards it, so its file stays in
> the commit log directory and its buffer is not released. {{awaitTermination}}
> only closes the segments in {{activeSegments}}.
> CASSANDRA-16925 introduced the condition when it moved the allocator onto
> {{Interruptible}}. The code it replaced held a {{shutdown}} flag that the
> allocator tested straight after {{createSegment}}, in the same loop pass, so
> the discard ran.
> The patch calls {{executor.shutdown()}}, which delivers one final
> {{SHUTTING_DOWN}} pass to the task and restores the discard.
> Patch:
> [mck/upstream/commitlog-allocator-shutdown/5.0|https://github.com/thelastpickle/cassandra/tree/mck/upstream/commitlog-allocator-shutdown/5.0]
> Provenance:
> [1976b540ca|https://github.com/datastax/cassandra/commit/1976b540ca8e63205f6d8a56aa0c7f4a6b2112e9]
> by [~brandon.williams]
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
