mcvsubbu commented on a change in pull request #4418: Fix potential resource
leak in the way we close a collection of closeables
URL: https://github.com/apache/incubator-pinot/pull/4418#discussion_r303634297
##########
File path:
pinot-core/src/main/java/org/apache/pinot/core/io/writer/impl/FixedByteSingleValueMultiColWriter.java
##########
@@ -99,9 +100,12 @@ public void setBytes(int row, int col, byte[] bytes) {
indexDataBuffer.readFrom(offset, bytes);
}
+ @Override
public void close()
throws IOException {
- this.indexDataBuffer.close();
- this.indexDataBuffer = null;
+ if (this.indexDataBuffer != null) {
Review comment:
It is a balance here.
Pinot query processing threads are not interruptable (at least now). The
parent thread lets the child thread run with the query while returning to the
broker after some timeout. It is therefore possible that a query thread is
using a segment (even after it has been released).
A retention or segment drop can cause the segment parts to be closed, and a
read request may come along, causing us to access bad memory, and a JVM crash
could result (we use direct access to memory, in unsafe manner)
NPE on a close is a much smaller evil, and I can live with that.
In the early days we even saw JVM crashes during segment unload.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
[email protected]
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
