-
-
Notifications
You must be signed in to change notification settings - Fork 138
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Don't close IonParser on EOF to be compatible with MappingIterator
when source is an empty InputStream
#487
Don't close IonParser on EOF to be compatible with MappingIterator
when source is an empty InputStream
#487
Conversation
…en source is an empty InputStream
@tgregg would assign this to you, but somehow github ui won't let me. It makes sense at high level but you might be more familiar with this part of code. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are actually two issues here:
- ion-java's reader should not throw
NullPointerException
ifclose()
is called more than once. Ensures that using a cursor after close has no effect. amazon-ion/ion-java#803 fixes that bug. That fix alone is enough to make yourtestReadFromEmpty
test pass. - The invocation of
IonParser.close()
withinIonParser.nextToken()
(which you proposed removing) is also suspect. IonParser implementsCloseable
(transitively), so callers should be invokingclose()
before an instance goes out of scope and not relying on the resource being closed as a side-effect of a different API. Furthermore, I noticed thatMappingIterator
has a_closeParser
option which it uses internally to determine whether to close its parser (in this case, an IonParser). An IonParser closing itself would seem to conflict with the intention of that option.
In summary, this change looks good to me, but I welcome @cowtowncoder to double-check my reasoning above. It can stand alone as a fix, and doesn't depend on the aforementioned ion-java fix.
@tgregg I agree with all above -- will merge. |
@yvrng So, looks good and I'd be happy to merge: there's just one process thing left: we'd need a CLA from: https://github.com/FasterXML/jackson/blob/master/contributor-agreement.pdf (unless you have already sent one -- just LMK if so and I can double-check). Once we have CLA I'll go ahead and merge this. CLA only needs to be sent once before the first contribution; usually easiest to print, fill & sign, scan/photo, email to Thank you again for submitting the patch! |
Thank you for your review and additional fixes. I've just sent the signed CLA document you asked for. |
CLA received, can now merge. |
MappingIterator
when source is an empty InputStream
…en source is an empty InputStream (#487)
Using a
IonObjectMapper
with aMappingIterator
to read ION objects fails when the providedInputStream
is empty, resulting in a "stream closed" exception on the first call onMappingIterator#hasNextValue()
.For any reason, the
ObjectReader
callsnextToken()
when initializing theMappingIterator
, and because thenextToken()
method callsclose()
when the token isnull
, this leads to the exception mentioned above.During my analysis to understand this error, I was unable to reproduce the "stream closed" exception (it seems to occur only when wrapping the source
InputStream
within aBufferedInputStream
), but I found aNullPointerException
under the same conditions.Here is an extract of the stack trace:
Everything seems to happen because the
InputStream
is closed on the first call, so I've removed the call toclose()
innextToken()
, assuming that the caller will close it.