Skip to content
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

Merged
merged 1 commit into from
Apr 26, 2024

Conversation

yvrng
Copy link
Contributor

@yvrng yvrng commented Apr 21, 2024

Using a IonObjectMapper with a MappingIterator to read ION objects fails when the provided InputStream is empty, resulting in a "stream closed" exception on the first call on MappingIterator#hasNextValue().

For any reason, the ObjectReader calls nextToken() when initializing the MappingIterator, and because the nextToken() method calls close() when the token is null, 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 a BufferedInputStream), but I found a NullPointerException under the same conditions.

Here is an extract of the stack trace:

java.lang.NullPointerException: Cannot read the array length because "b" is null

	at java.base/java.io.ByteArrayInputStream.read(ByteArrayInputStream.java:175)
	at com.amazon.ion.impl.IonCursorBinary.refill(IonCursorBinary.java:672)
	at com.amazon.ion.impl.IonCursorBinary.fillAt(IonCursorBinary.java:537)
	at com.amazon.ion.impl.IonCursorBinary.slowMakeBufferReady(IonCursorBinary.java:1013)
	at com.amazon.ion.impl.IonCursorBinary.slowNextToken(IonCursorBinary.java:1599)
	at com.amazon.ion.impl.IonCursorBinary.slowOverflowableNextToken(IonCursorBinary.java:1688)
	at com.amazon.ion.impl.IonCursorBinary.slowNextValue(IonCursorBinary.java:1742)
	at com.amazon.ion.impl.IonCursorBinary.nextValue(IonCursorBinary.java:1720)
	at com.amazon.ion.impl.IonReaderContinuableCoreBinary.nextValue(IonReaderContinuableCoreBinary.java:460)
	at com.amazon.ion.impl.IonReaderContinuableApplicationBinary.nextValue(IonReaderContinuableApplicationBinary.java:909)
	at com.amazon.ion.impl.IonReaderContinuableTopLevelBinary.next(IonReaderContinuableTopLevelBinary.java:168)
	at com.fasterxml.jackson.dataformat.ion.IonParser.nextToken(IonParser.java:672)
	at com.fasterxml.jackson.databind.MappingIterator.hasNextValue(MappingIterator.java:246)

Everything seems to happen because the InputStream is closed on the first call, so I've removed the call to close() in nextToken(), assuming that the caller will close it.

@cowtowncoder
Copy link
Member

@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.

Copy link
Contributor

@tgregg tgregg left a 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:

  1. ion-java's reader should not throw NullPointerException if close() 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 your testReadFromEmpty test pass.
  2. The invocation of IonParser.close() within IonParser.nextToken() (which you proposed removing) is also suspect. IonParser implements Closeable (transitively), so callers should be invoking close() 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 that MappingIterator 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.

@cowtowncoder
Copy link
Member

@tgregg I agree with all above -- will merge.

@cowtowncoder cowtowncoder added the cla-needed PR looks good (although may also require code review), but CLA needed from submitter label Apr 24, 2024
@cowtowncoder
Copy link
Member

@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 cla at fasterxml dot com.

Thank you again for submitting the patch!

@yvrng
Copy link
Contributor Author

yvrng commented Apr 24, 2024

Thank you for your review and additional fixes.

I've just sent the signed CLA document you asked for.

@cowtowncoder
Copy link
Member

CLA received, can now merge.

@cowtowncoder cowtowncoder removed the cla-needed PR looks good (although may also require code review), but CLA needed from submitter label Apr 26, 2024
@cowtowncoder cowtowncoder merged commit 2f6f1fd into FasterXML:2.18 Apr 26, 2024
4 checks passed
@cowtowncoder cowtowncoder changed the title Don't close IonParser on EOF to be compatible with MappingIterator when source is an empty InputStream Don't close IonParser on EOF to be compatible with MappingIterator when source is an empty InputStream Apr 26, 2024
tatu-at-datastax pushed a commit that referenced this pull request Apr 26, 2024
tatu-at-datastax added a commit that referenced this pull request Apr 26, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants