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

JsonParser should not resume or pause the source wrapped read stream when it is ended #2790

Closed
abelhegedus opened this issue Jan 3, 2019 · 6 comments
Labels
Milestone

Comments

@abelhegedus
Copy link

abelhegedus commented Jan 3, 2019

We have implemented streaming Json from an AsyncFile as shown in this example. This has worked nicely before 3.6.0 but now results in a java.lang.IllegalStateException: File handle is closed in 3.6.2.

Based on some debugging, the following happens:

  1. io.vertx.core.parsetools.impl.JsonParserImpl#end calls io.vertx.core.parsetools.impl.JsonParserImpl#checkPending which will call the endHandler.
  2. endHandler closes the AsyncFile
  3. io.vertx.core.streams.ReadStream#resume is called (on the already closed stream)
  4. the exceptionHandler is called with the above exception

As far as I can see, issue #2584 and PR #2583 seems to be related.

Is the example outdated? Should we handle file closing in some other way?

@vietj
Copy link
Member

vietj commented Jan 3, 2019

can you provide a reproducer for the issue ?

@vietj
Copy link
Member

vietj commented Jan 3, 2019

what is actually calling resume ? it is not clear

@abelhegedus
Copy link
Author

what is actually calling resume ? it is not clear

I have added stable links to the original comments.

can you provide a reproducer for the issue ?

I can do that, though I think the linked example should reproduce the issue as well.

@vietj
Copy link
Member

vietj commented Jan 4, 2019

ok, we will check the example

@vietj vietj added this to the 3.6.3 milestone Jan 4, 2019
@vietj vietj added the bug label Jan 4, 2019
@KowalczykBartek
Copy link
Contributor

Just wanted to let you know :) making this small change in io.vertx.core.parsetools.implJsonParserImpl

if (currentToken == null) {
  if (ended) {
    if (endHandler != null) {
      endHandler.handle(null);
    }
    return; // <-- here we need to skip rest of the method if file reading is ended
  }
  break;
}

allows to read entire file (and AsyncFileImpl is also closed). Unfortunately there is no tests for parser implementation so I am not sure whether it doesn't breaks something other :|

@vietj
Copy link
Member

vietj commented Jan 9, 2019

there is io.vertx.core.parsetools.JsonParserTest also it is needed to add a new test borrowed from the example that fails

KowalczykBartek added a commit to KowalczykBartek/vert.x that referenced this issue Jan 9, 2019
Signed-off-by: Kowalczyk <bkowalczyyk@gmail.com>
KowalczykBartek added a commit to KowalczykBartek/vert.x that referenced this issue Jan 10, 2019
Signed-off-by: Kowalczyk <bkowalczyyk@gmail.com>

Signed-off-by: Kowalczyk <bkowalczyyk@gmail.com>
KowalczykBartek added a commit to KowalczykBartek/vert.x that referenced this issue Jan 10, 2019
Signed-off-by: Kowalczyk <bkowalczyyk@gmail.com>

Signed-off-by: Kowalczyk <bkowalczyyk@gmail.com>

Signed-off-by: Kowalczyk <bkowalczyyk@gmail.com>
KowalczykBartek added a commit to KowalczykBartek/vert.x that referenced this issue Jan 11, 2019
Signed-off-by: Kowalczyk <bkowalczyyk@gmail.com>

Signed-off-by: Kowalczyk <bkowalczyyk@gmail.com>
KowalczykBartek added a commit to KowalczykBartek/vert.x that referenced this issue Jan 11, 2019
Signed-off-by: Kowalczyk <bkowalczyyk@gmail.com>

Signed-off-by: Kowalczyk <bkowalczyyk@gmail.com>

Signed-off-by: Kowalczyk <bkowalczyyk@gmail.com>
KowalczykBartek added a commit to KowalczykBartek/vert.x that referenced this issue Jan 11, 2019
Signed-off-by: Kowalczyk <bkowalczyyk@gmail.com>

Signed-off-by: Kowalczyk <bkowalczyyk@gmail.com>

Signed-off-by: Kowalczyk <bkowalczyyk@gmail.com>
KowalczykBartek added a commit to KowalczykBartek/vert.x that referenced this issue Jan 11, 2019
Signed-off-by: Kowalczyk <bkowalczyyk@gmail.com>

Signed-off-by: Kowalczyk <bkowalczyyk@gmail.com>
KowalczykBartek added a commit to KowalczykBartek/vert.x that referenced this issue Jan 13, 2019
Provide fix and test for eclipse-vertx#2790

Signed-off-by: Kowalczyk <bkowalczyyk@gmail.com>

Signed-off-by: Kowalczyk <bkowalczyyk@gmail.com>
vietj pushed a commit that referenced this issue Jan 14, 2019
Provide fix and test for #2790

Signed-off-by: Kowalczyk <bkowalczyyk@gmail.com>

Signed-off-by: Kowalczyk <bkowalczyyk@gmail.com>
@vietj vietj changed the title JsonParser with AsyncFile throws IllegalStateException after calling endHandler JsonParser should not resume or pause the source wrapped read stream when it is ended Jan 14, 2019
@vietj vietj mentioned this issue Jan 14, 2019
24 tasks
vietj pushed a commit that referenced this issue Jan 14, 2019
Provide fix and test for #2790

Signed-off-by: Kowalczyk <bkowalczyyk@gmail.com>

Signed-off-by: Kowalczyk <bkowalczyyk@gmail.com>
@vietj vietj closed this as completed Jan 14, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants