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

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

Open
simonbasle opened this issue Nov 24, 2023 · 12 comments
Labels
2.17 Issues planned (at earliest) for 2.17 has-failing-test Indicates that there exists a test case (under `failing/`) to reproduce the issue

Comments

@simonbasle
Copy link

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
Copy link
Member

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

@simonbasle
Copy link
Author

simonbasle commented Nov 24, 2023

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
Copy link
Member

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
Copy link
Member

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
Copy link
Author

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
Copy link
Member

@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
Copy link
Member

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
Copy link
Author

simonbasle commented Jan 15, 2024

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
Copy link
Member

cowtowncoder commented Jan 15, 2024

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
Copy link
Member

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
Copy link
Member

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 cowtowncoder added the has-failing-test Indicates that there exists a test case (under `failing/`) to reproduce the issue label Jan 15, 2024
@cowtowncoder
Copy link
Member

cowtowncoder commented Jan 15, 2024

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)

cowtowncoder added a commit that referenced this issue Jan 15, 2024
cowtowncoder added a commit that referenced this issue Jan 15, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
2.17 Issues planned (at earliest) for 2.17 has-failing-test Indicates that there exists a test case (under `failing/`) to reproduce the issue
Projects
None yet
Development

No branches or pull requests

3 participants