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

Stream goes into flowing mode immediately #66

Open
LaurensRietveld opened this issue Jun 10, 2020 · 5 comments
Open

Stream goes into flowing mode immediately #66

LaurensRietveld opened this issue Jun 10, 2020 · 5 comments
Labels
bug Something isn't working

Comments

@LaurensRietveld
Copy link

LaurensRietveld commented Jun 10, 2020

We've experienced some odd issues that are probably caused by race conditions. I think it boils down to the stream going into flowing mode immediately

Afaik, the stream should only go into flowing mode when:

  • We attach an on-data listener
  • We pipe it to another writable stream
  • We explicitly call resume()

I would expect the below snippet to terminate fast and essentially be a no-op. In reality it isn't, and depending on the size of test.jsonld it will take a while for it to finish.

const fs = require("fs");
const { JsonLdParser } = require("./");
fs.createReadStream("./test.jsonld")
  .pipe(
    new JsonLdParser({
      baseIRI: "http://base"
    })
  )
  //Not registering an on-data listener, so not expecting this stream to start flowing
  //.on('data', () => {})

Would you agree with my observation with the stream going into flowing mode too quickly?

@rubensworks
Copy link
Owner

I did not explicitly test such cases, so the parser may indeed be too eager with its parsing tasks. Thanks for reporting!

@Tpt
Copy link
Contributor

Tpt commented Aug 17, 2022

I have not managed to reproduce this bug. It seems to me that both in streaming and in regular mode the parser used as a Transform node and the import method do not start flowing immediatly. I wrote tests in #99 to check this behavior.

@rubensworks
Copy link
Owner

@Tpt Might be interesting to also check for the case where element by element is being read (.read()), to ensure it does not enter flowing mode (explicitly or implicitly).

But in any case, it's possible that this specific issue was already fixed by some other change.

@Tpt
Copy link
Contributor

Tpt commented Aug 17, 2022

@Tpt Might be interesting to also check for the case where element by element is being read (.read()), to ensure it does not enter flowing mode (explicitly or implicitly).

I have updated #99 with tests. It seems to work properly.

@rubensworks
Copy link
Owner

Ok, great!
Not sure if we're handling all cases by only checking readableFlowing though.
Specifically, when streamingProfile is true, then the JSON should be read chunk by chunk.
So we must be sure that the underlying stream is not being read further than required. (I guess this overlaps with #71)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

3 participants