GH-3628: prevent NPE & unclosed releaser#3629
Conversation
| store.addColumn(COLUMN, newReaderWithoutPages(options)); | ||
|
|
||
| // setReleaser() is intentionally NOT called here. | ||
| store.close(); |
There was a problem hiding this comment.
having AssertJ (#3616) would make testing this a bit easier and more explicit, because we could then just say assertThatCode(store::close).doesNotThrowAnyException()
|
|
||
| try { | ||
| store.close(); | ||
| throw new AssertionError("Expected close() to propagate the reader failure"); |
There was a problem hiding this comment.
AssertJ would also simplify testing this error condition. We would end up having assertThatThrownBy(store::close).isInstanceof(...).hasMessage(...)
| for (ColumnChunkPageReader reader : readers.values()) { | ||
| reader.releaseBuffers(); | ||
| try { | ||
| for (ColumnChunkPageReader reader : readers.values()) { |
There was a problem hiding this comment.
Can we keep closing the remaining readers if one releaseBuffers() call fails? As written, a failure still leaves later readers unreleased, and a releaser.close() failure would hide the original exception.
There was a problem hiding this comment.
that's a good point. I've slightly updated the code so that any occurring failure is being kept and shows up in the final exception that is being thrown
There was a problem hiding this comment.
also note that AutoCloseables will correctly deal with a null
2 participants
Rationale for this change
ColumnChunkPageReadStore.close() NPEs if setReleaser() was never called. Also it's possible that the
releaseritself isn't closed when releasing the buffers of the readers fails anywhere in the loopCloses #3628
What changes are included in this PR?
Are these changes tested?
added a test that reproduces both cases
Are there any user-facing changes?