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.
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.
I wrote a bit on that point here datastax#1103 (comment).
Note that it doesn't look like the
Closeclass defined in thePartitionIterators#getOnlyElementmethod 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.
Just tested again, and I was wrong, that method is called. Sorry for the confusion.
There was a problem hiding this comment.
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.
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.
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.