Close the underlying input stream safely #1
Conversation
This commit includes the following changes: - Close the underlying input stream only if it's not being read. - Always read to `readAheadBuffer`.
| activeBuffer.flip(); | ||
| readAheadBuffer.position(0); | ||
| readAheadBuffer.flip(); | ||
| long skippedFromInputStream = underlyingInputStream.skip(toSkip); |
There was a problem hiding this comment.
This does not look right, we should be reading into the activeBuffer instead?
There was a problem hiding this comment.
I changed your codes to always write to readAheadBuffer so that it's easy to know which one we are writing to. readInternal will handle the case that activeBuffer is empty.
| } | ||
| // Flip this so that the close method will not close the underlying input stream when we | ||
| // are reading. | ||
| isReading = true; |
There was a problem hiding this comment.
Why do we need this variable? Can we use readInProgress instead?
There was a problem hiding this comment.
readInProgress == true means the task will be submitted. But it may not run if the executor service is shut down. I changed shutdown to shutdownNow in order to interrupt the blocking read.
There was a problem hiding this comment.
I see. Instead of doing that should we just call waitForAsyncReadComplete in close() function call so that we can safely close the input stream? It would clean up the code a lot and make it less complicated.
There was a problem hiding this comment.
waitForAsyncReadComplete can be interrupted as well.
## What changes were proposed in this pull request? There were two related fixes regarding `from_json`, `get_json_object` and `json_tuple` ([Fix #1](apache@c8803c0), [Fix apache#2](apache@86174ea)), but they weren't comprehensive it seems. I wanted to extend those fixes to all the parsers, and add tests for each case. ## How was this patch tested? Regression tests Author: Burak Yavuz <[email protected]> Closes apache#20302 from brkyvz/json-invfix.
2 participants
This commit includes the following changes:
readAheadBuffer.