FilteringParserDelegate can go into an infinite loop if underlying parser is non-blocking

[simonbasle]

Jackson version (BOM): 2.15.2

While experimenting with JsonPointer in Spring Framework, I tried to build up on the existing FilteringParserDelegate and associated JsonPointerBasedTokenFilter. Unfortunately, this doesn't seem to work with a non-blocking JsonParser delegate, for several reasons:

• it doesn't delegate method canParseAsync() (ends up returning the default interface implem => false)
• it doesn't delegate method getNonBlockingInputFeeder() (same as above => null)
• it isn't aware of JsonToken.NOT_AVAILABLE

While the first two are easily circumvented by extending FilteringParserDelegate, the last one is the truly problematic one. It results in going down the code path of a "scalar value", and will continuously call delegate.nextToken() which continues to result in NOT_AVAILABLE.

This leads to an infinite loop when attempting to filter based on a JsonPointer on top of a non-blocking parser.

Is there anything fundamentally preventing FilteringParserDelegate from being compatible with non-blocking parsers that I might have overlooked? Otherwise I think it's pretty close, hopefully that can be fixed 😄

[pjfanning]

Can you provide reproducible test cases? Volunteers appreciate time savers like that.

[simonbasle]

Sure! Here is a testcase, probably needs some refinement (especially around asserting timeout with assertj).
Please note that JUnit Timeout rule doesn't work to cut the loop short if the call isn't offloaded to another thread.

    public void testFilteringNonBlockingParser() throws IOException {
        JsonFactory factory = new JsonFactoryBuilder().build();
        JsonParser nonBlockingParser = factory.createNonBlockingByteArrayParser();
        ByteArrayFeeder inputFeeder = (ByteArrayFeeder) nonBlockingParser.getNonBlockingInputFeeder();
        String json = "{\"first\":1,\"second\":2}";
        byte[] jsonBytes = json.getBytes(StandardCharsets.UTF_8);

        JsonParser filteringParser = new FilteringParserDelegate(nonBlockingParser, new JsonPointerBasedFilter("/second"),
                TokenFilter.Inclusion.ONLY_INCLUDE_ALL, false);

        ScheduledExecutorService executor = Executors.newSingleThreadScheduledExecutor();
        executor.schedule(() -> {
            try {
                inputFeeder.feedInput(jsonBytes, 0, jsonBytes.length);
            } catch (IOException e) {
                throw new RuntimeException(e);
            }
        }, 500, TimeUnit.MILLISECONDS);

        Future<JsonToken> future = Executors.newSingleThreadExecutor().submit(() -> filteringParser.nextToken());
        Assertions.assertThat(future)
                .succeedsWithin(Duration.ofSeconds(5))
                .isNotNull()
                .isNotEqualTo(JsonToken.NOT_AVAILABLE);
    }

[cowtowncoder]

Quick note: yes, first 2 seem like easy enough oversights to fix (alas, I don't know of a way to properly prevent the issue of missing overrides).

But I suspect you may be right in that handling JsonToken.NOT_AVAILABLE is rather trickier challenge.

Let's start with #1146 tho, add overrides where they belong (JsonParserDelegate).

[cowtowncoder]

Ok, so... my first instinct is that handling of JsonToken.NOT_AVAILABLE (which existing FilteringParserDelegate isn't designed to do at all, alas, since it I think pre-dates async parser choice) requires a lot of changes.
But a plus side is that since this token is not produced by blocking parsers, changes needed should be safe (i.e. if we can make async parser work fine, blocking should too).

The reason I think handling is requires all over the place is that basically NOT_AVAILABLE can occur at any point within document. Similar to how async parser itself has to be available to retain state no matter where available input happens to end, within or between tokens etc.
So it is not a simple matter of just checking it where looping occurs.
Further, JsonToken.NOT_AVAILABLE needs to be exposed to caller since that signals need to feed more input.

But... I am not convinced it would be impossible to do this. :)

@simonbasle If you have time to try to add changes, I'd be happy to review code. One thing that would be very important, I think, would be to add test cases that feed input (byte-by-byte, in bigger chunks); support for this exists, see tests in com.fasterxml.jackson.core.json.async.
I am not sure I'll have time to work on solution myself, but would love to help if you have time and interest.

[simonbasle]

hey @cowtowncoder, thanks for the analysis. unfortunately I don't think I will have the possibility to try to add changes anytime soon 😞

hopefully there is some kind of low hanging fruit where the parsing doesn't work but fails fast ? (e.g. throwing an exception if the underlying parser is returning NOT_AVAILABLE ? or even if the parser passed in the constructor is async ?)
this wouldn't really help in my case (async parsing of merely a sub-set of a document, pointed at by a JsonPointer), but at least it would avoid tricky livelock situations.

[cowtowncoder]

@simonbasle All legit, and also bit challenging to do... esp. since patch release typically should not change behavior minus bug fixes. And while there are tests, I think due to complexity of filtering delegation, there are still gaps.

For patch release, ideally I do think NOT_AVAILABLE should be recognized... yet that's not a small change probably.
On refusing constructing for async-parser (there is a method to call to check fortunately) -- and maybe having alternate constructor/factory method that would allow it? (since one can, in theory, make it all work as long as all content for a doc is passed immediately) -- that'd have to go in a new minor version (2.17).

I'd like to improve things here, and maybe I can. But it does need to fight wrt all other priorities.

But I'll keep this on my ever-expanding todo-list.

[cowtowncoder]

Quick notes:

1. I feel less confident about how to tackle failing construction of FilteringParserDelegate: I realized there are no factory methods, and checking in constructor seems difficult to document. I guess combination of failing on existing constructors, along with adding factory method that allows might work. Feedback welcome
2. Changes definitely need to be in new minor version (at this point 2.17), not patch

[simonbasle]

I guess combination of failing on existing constructors, along with adding factory method that allows might work

Not sure what you mean by that. That said I can understand the apprehensiveness of failing the constructor and how to document that.

If you feel the risk is too big in terms of impact on existing users, I guess a big WARN logging would perhaps be the next best thing 🤷‍♂️
But from my perspective there is currently no point in accepting a non-blocking parser in a constructor nor in a factory method, since that will inevitably lead to infinite looping.

Changes definitely need to be in new minor version (at this point 2.17), not patch

👍

[cowtowncoder]

Ok, let me start by explaining one possibly valid use case for Async parser: if the whole content is indeed read into buffer, it seems possible to use filtering parser without issue (in this case NOT_AVAILABLE is never reported).
This is why I think it might be good to have a way to explicitly construct instance backed by Async parser.
Without such use case I agree there's no point.

Looking at tests, there is:

com.fasterxml.jackson.core.json.async.AsyncTokenFilterTest

which actually verifies that usage works (assuming content is indeed all available).

As to logging WARNING: Jackson does zero-logging, as matter of principle (well, some modules like Afterburner do log one line for specific failure) so that is not available as a technique. But I am not sure it'd be good way even if we could do that.

But I think there might be another way: create method for validating given parser -- overridable -- which will by default fail on async parser. I think I;ll go with that.

[cowtowncoder]

Forgot to say: thank you @simonbasle for both reporting this and going through with some of the options.

Beyond initial blocking of usage, it'd be nice to add handling of NOT_AVAILABLE to make it either work to some degree (if feasible... not sure it is), or, if not, adding explicit failure. With that we could perhaps just remove initially added block.

[cowtowncoder]

Ok. So, as per AsyncTokenFilterTest, usage is to be allowed. But there' background to this, #462 / #463 which should prevent infinite loop on skipChildren(). But that is obviously just partial resolution.

This does lead to the obvious problem: should we block supported usage (as per test) to prevent rather nasty issues for "bad" usage?

I guess I should add test from here to at least reproduce failure case.

[cowtowncoder]

Adding test which does reproduce problem, but cannot quite pinpoint where. Need to see how to debug where the infinite loop occurs (I realize there are probably a few places... but easier to find one etc)

[iifawzi]

I'm trying to make my head around what need to be achieved here.
As per what I understand, the ultimate goal is to be able to filter for a specific token while using non-blocking parser, no?

After debugging for a bit, the first infinite loop that I caught occurs because nextToken @ FilteringParserDelegate isn't aware of the NOT_AVAILABLE token. Why isn't handing this enough? If not available token, we either return null or not_available itself as an indication for the client to complete the input. then next time the nextToken is used, if the token exist, it will be returned, no? (I need to verify that)

Second point: I feel like the test itself is problematic,

jackson-core/src/test/java/com/fasterxml/jackson/core/tofix/async/FilteringOnAsyncInfiniteLoop1144Test.java

Lines 63 to 70 in 0b21629

	executor.schedule(() -> {
	try {
	// This doesn't seem to make a difference:
	inputFeeder.feedInput(DOC, 0, DOC.length);
	} catch (Exception e) {
	throw new RuntimeException(e);
	}
	}, 500, TimeUnit.MILLISECONDS);

If we're feeding in a separate thread after a delay, how do we expect the assertion to succeed with no input (The assertion will run before the input is fed)? (Unless the goal of the filter delegate to keep looking until input is fed, and token is found?)

Looking forward to your insights, as I feel like I'm missing something critical here.
@cowtowncoder

[cowtowncoder]

Re-reading what I said earlier, I think that my thinking was that:

1. It is indeed NOT possible (or at least practical) to make things work transparently.
2. However: some not-really-async usage (where input is all fed at once, so no JsonToken.NOT_AVAILABLE ever returned)
3. Since we cannot distinguish ok vs not-ok cases, it'd be good to make existing construction cases explicitly fail with exception, if given async-parser but
4. To support future "ok-because-I-know-it-is" usage, new construction/usage method(s) added for
5. As an addition or alternative to (3) we should also make encountering of JsonToken.NOT_AVAILABLE just fail (if it doesn't already).

As to test, executor; I'm pretty sure that was just meant to bail out from test when infinite loop occurs.

Does this make more sense?

[iifawzi]

Hey, thanks @cowtowncoder the direction and goal is more clear now.
Will see if I can create an initial PR this week with this proposal