-
Notifications
You must be signed in to change notification settings - Fork 3.6k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
CASSANDRA-19624: ModificationStatement#casInternal leaks RowIterator #3296
base: cassandra-4.0
Are you sure you want to change the base?
CASSANDRA-19624: ModificationStatement#casInternal leaks RowIterator #3296
Conversation
Fixes https://issues.apache.org/jira/browse/CASSANDRA-19624 This patch closes the RowIterator created in the ModificationStatement#casInternal method, preventing the leak of resources initialized for the RowIterator.
@@ -652,9 +652,11 @@ static RowIterator casInternal(CQL3CasRequest request, long timestamp, int nowIn | |||
SinglePartitionReadQuery readCommand = request.readCommand(nowInSeconds); | |||
FilteredPartition current; | |||
try (ReadExecutionController executionController = readCommand.executionController(); | |||
PartitionIterator iter = readCommand.executeInternal(executionController)) | |||
PartitionIterator iter = readCommand.executeInternal(executionController); | |||
RowIterator rowIterator = PartitionIterators.getOnlyElement(iter, readCommand)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, one thing I'm wondering - is it possible that we may cause a double close of the iter
(PartitionIterator
) as the returned row iterator includes a "close transformation"? If so, is it a problem?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wrote a bit on that point here datastax#1103 (comment).
Note that it doesn't look like the
Close
class defined in thePartitionIterators#getOnlyElement
method is ever called. I'm not sure if that's an issue or not. However, I added some basic logging to the chunk cache during my testing, and I can see that this PR's change fixes a reference leak.
In my quick testing, I found that the PartitionIterator
wasn't closed via the close transformation. It is possible that my debugging wasn't representative, as I am somewhat new to this code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just tested again, and I was wrong, that method is called. Sorry for the confusion.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I removed the iter
from the try-with-resources block, so it won't be double closed now. I can see with code coverage that it is getting run via the PreparedStatementsTest
. Resolving this for now, let me know if it needs to change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Though, we're at risk of leaking the PartitionIterator
in the event of an exception in the PartitionIterators.getOnlyElement
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I know, all in all your patch fixes a leak which was occurring all the time, leaving a leak occurring only in case of a failure :-) I suppose this is not the only place vulnerable to that problem, in other words, perhaps it can be a subject of another ticket.
I confirmed via code coverage that this code is exercised by the PreparedStatementsTest class.
Fixes https://issues.apache.org/jira/browse/CASSANDRA-19624
This patch closes the RowIterator created in the ModificationStatement#casInternal method, preventing the leak of resources initialized for the RowIterator.
This is my first C* patch, please let me know if I need to change anything.