You are viewing a plain text version of this content. The canonical link for it is here.
Posted to pr@cassandra.apache.org by "iamaleksey (via GitHub)" <gi...@apache.org> on 2026/06/10 10:42:16 UTC
Re: [PR] putShortVolatile is not volatile in InMemoryTrie [cassandra]
iamaleksey commented on code in PR #4820:
URL: https://github.com/apache/cassandra/pull/4820#discussion_r3387145889
##########
src/java/org/apache/cassandra/io/sstable/AbstractSSTableIterator.java:
##########
@@ -97,6 +97,7 @@ protected AbstractSSTableIterator(SSTableReader sstable,
}
else
{
+ Reader reader = null;
Review Comment:
This introduction of a local variable doesn't seem to meaningfully affect anything? Not a big deal and I don't mind it too much, but if I were to write I'd just leave it be to minimize the disturbance to `git blame`.
##########
src/java/org/apache/cassandra/io/sstable/AbstractSSTableIterator.java:
##########
@@ -140,6 +144,19 @@ protected AbstractSSTableIterator(SSTableReader sstable,
catch (IOException e)
{
sstable.markSuspect();
+
+ if (reader != null)
+ {
+ try
+ {
+ reader.close();
+ }
+ catch (IOException suppressed)
+ {
+ e.addSuppressed(suppressed);
+ }
+ }
+
String filePath = file.getPath();
if (shouldCloseFile)
Review Comment:
This will double close the file now if `shouldCloseFile == true` (once here, and once in the newly added block above). Perhaps change the condition to `if (reader == null && shouldCloseFile)`? Now, this particular `FileDataInput` is a `RAR`, whose `close()` impl is idempotent, so the double close won't break anything, but still.
##########
src/java/org/apache/cassandra/io/tries/ReverseValueIterator.java:
##########
Review Comment:
So, both here and for `ValueIterator` it would probably be better to wrap the entire if-else block in the constructors. So that if e.g. `limit.next()` call throws, we still clean up. It would also reduce the visual nesting where it matters more.
##########
src/java/org/apache/cassandra/io/sstable/AbstractSSTableIterator.java:
##########
@@ -140,6 +144,19 @@ protected AbstractSSTableIterator(SSTableReader sstable,
catch (IOException e)
{
sstable.markSuspect();
+
+ if (reader != null)
+ {
+ try
+ {
+ reader.close();
+ }
+ catch (IOException suppressed)
+ {
+ e.addSuppressed(suppressed);
+ }
+ }
+
String filePath = file.getPath();
if (shouldCloseFile)
Review Comment:
Or set `shouldCloseFile = false` after `reader.close()` above.
##########
src/java/org/apache/cassandra/io/sstable/AbstractSSTableIterator.java:
##########
@@ -140,6 +144,19 @@ protected AbstractSSTableIterator(SSTableReader sstable,
catch (IOException e)
Review Comment:
`UnfilteredValidation.handleInvalid()` can throw a `CorruptSSTableException` which is an `RTE` so it wouldn't be caught here and we'd still have a leak.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org