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

Error: stream.push() after EOF #207

Closed
will-holley opened this issue May 5, 2016 · 17 comments
Closed

Error: stream.push() after EOF #207

will-holley opened this issue May 5, 2016 · 17 comments

Comments

@will-holley
Copy link

A heads up.

Error: stream.push() after EOF
at readableAddChunk (/scraper/node_modules/slice-stream/node_modules/readable-stream/lib/_stream_readable.js:168:15)
at SliceStream.Readable.push (/scraper/node_modules/slice-stream/node_modules/readable-stream/lib/_stream_readable.js:149:10)
at SliceStream.Transform.push (/scraper/node_modules/slice-stream/node_modules/readable-stream/lib/_stream_transform.js:145:32)
at SliceStream.sliceFn (/scraper/node_modules/pullstream/pullstream.js:81:14)
at SliceStream._transform (/scraper/node_modules/slice-stream/slicestream.js:28:10)
at SliceStream.Transform._read (/scraper/node_modules/slice-stream/node_modules/readable-stream/lib/_stream_transform.js:184:10)
at SliceStream.Transform._write (/scraper/node_modules/slice-stream/node_modules/readable-stream/lib/_stream_transform.js:172:12)
at doWrite (/scraper/node_modules/slice-stream/node_modules/readable-stream/lib/_stream_writable.js:237:10)
at clearBuffer (/scraper/node_modules/slice-stream/node_modules/readable-stream/lib/_stream_writable.js:316:5)
at onwrite (/scraper/node_modules/slice-stream/node_modules/readable-stream/lib/_stream_writable.js:274:7)
at WritableState.onwrite (/scraper/node_modules/slice-stream/node_modules/readable-stream/lib/_stream_writable.js:106:5)
at afterTransform (/scraper/node_modules/slice-stream/node_modules/readable-stream/lib/_stream_transform.js:104:5)
at TransformState.afterTransform (/scraper/node_modules/slice-stream/node_modules/readable-stream/lib/_stream_transform.js:79:12)
at SliceStream._transform (/scraper/node_modules/slice-stream/slicestream.js:29:5)
at SliceStream.Transform._read (/scraper/node_modules/slice-stream/node_modules/readable-stream/lib/_stream_transform.js:184:10)
at SliceStream.Readable.read (/scraper/node_modules/slice-stream/node_modules/readable-stream/lib/_stream_readable.js:362:10)
@calvinmetcalf
Copy link
Contributor

ok...this is likely an issue with something using readable stream that ended the stream and then wrote more to it

@mcollina
Copy link
Member

Closing for no activity.

@mauceri
Copy link

mauceri commented Jun 12, 2017

I believe you should reopen this case.
Imagine you have a _transform() like this:

   _transform(chunk, encoding, done) {
        let data = chunk.toString();
        this.rest += data;
        [this.toPush, this.rest] = this.f(this.rest);
        for (let i = 0; i < this.toPush.length; i++) {
            if (!this.push(this.toPush[i])) {
                this._source.readStop();
                break;
            } 
        }
        done()
    }

where f is, for instance, a function spliting the received chunk in paragraphs. rest is something at the end of the chunk f cannot determine if it is an entire paragraphe, and thus expects more data (another chunk). When everything has been read one can assume rest is an entire paragraph, and then _flush is used to push it as below. The exception described by Vivism is the thrown, likely because "<p>"+this.rest+"</p>" is greater then this.rest. It's not really the expected behavior...

    _flush(done) {
        if (this.rest !== "") this.push("<p>"+this.rest+"</p>");
        this.rest = null;
        this.toPush = null;
        done()

    }

@calvinmetcalf
Copy link
Contributor

If I'm reading your example right that isn't an issue with streams as much as a bug in the userland code

@mauceri
Copy link

mauceri commented Jun 12, 2017

Why? It's a little bit short as an answer... Why do you think this.push("<p>"+this.rest+"</p>"); should trigger an exception?

@mauceri
Copy link

mauceri commented Jun 12, 2017

Is the example given in chapter 5 IMPLEMENTING TRANSFORM STREAMS of the book Node.js Design Patterns - Second Edition also wrong and this article as well?

@mauceri
Copy link

mauceri commented Jun 12, 2017

When you say 'ok...this is likely an issue with something using readable stream that ended the stream and then wrote more to it' does it mean this _flush method is useless?

@mauceri
Copy link

mauceri commented Jun 12, 2017

And last question: why this code does not trigger any exception?

    _flush(done) {
        if (this.rest !== "") this.push(this.rest);
        this.rest = null;
        this.toPush = null;
        done()

    }

@calvinmetcalf
Copy link
Contributor

this.rest is not something that streams care about so I suspect the root issue lies somewhere else, can you post a enough code that I can run it locally?

@mauceri
Copy link

mauceri commented Jun 12, 2017

This code works fine:

const {Transform} = require('stream');

class FSAReader extends Transform {
    constructor(f) {
        super();
        this.f = f;
        super();
        this.toPush = null;
        this.rest = "";
    }

    _transform(chunk, encoding, done) {
        let data = chunk.toString();
        this.rest += data;
        [this.toPush, this.rest] = this.f(this.rest);


        for (let i = 0; i < this.toPush.length; i++) {
            //console.log("Try to push "+this.toPush[i]);
            if (!this.push(this.toPush[i])) {
                //this._source.readStop();
                break;
            } else {
                //console.log(this.toPush[i]+" pushed");
            }
        }
        console.log("*********"+this.rest);
        done()
    }

    _flush(done) {
        if (this.rest !== "") this.push(this.rest);
        this.rest = null;
        this.toPush = null;
        done()

    }

}

module.exports = FSAReader;

the test is here:

describe('FSAReader', function () {
    it('Simple stupid test' , function () {
        let dataPath = path.join(__dirname, '..', 'resources', 'heartOfDarkness.txt');

        console.log(dataPath);
        let source = fs.createReadStream(dataPath);

        let f = function(d) {
            let re = /\n\n/;
            let lp = d.split(re);
            let res = []

            let rest = lp[lp.length - 1];
            lp = lp.slice(0,lp.length - 1);
            res = lp.map(p => {return "<p>"+p+"</p>"});

            return [res, rest]
        };

        let r = new FSAReader(f);
        
        source.pipe(r);

        let res1= f("abcd\n\naaaazcd\n\naaaarcd\n\naaa\n");
        console.log(res1[0][0]);
        console.log(res1[0]);
        console.log(res1[1]);

        r.on('readable', function () {
            let rres;

            while (null !== (rres = r1.read())) {
                console.log(""+rres);
            }
        });
    });

but if I change the _flush method in what I gave first, it fails... (It throws the exception)

I commented //this._source.readStop(); in order to be sure it cannot interfere.

@calvinmetcalf
Copy link
Contributor

let re = /\n\n/;

should be

let re = /\r?\n\r?\n/;

and there is an extra super call

both versions work for me once I fix those things (the version with the other regex does work but it just wraps the whole thing in a single <p> tag.

also you can simplify your transform into

  let data = chunk.toString();
        this.rest += data;
        [this.toPush, this.rest] = this.f(this.rest);

        for (let thing of this.toPush) {
          this.push(thing);
        }
        console.log("*********"+this.rest);
        done()

@mauceri
Copy link

mauceri commented Jun 12, 2017

Thanks, but it does not help. I'm on a Mac and I do not have the LF, CR problem, but I made the changes to see, and I got the same exception (BTW I do not understand why you think some syntactic sugar can have any incidence on the expected result). Maybe it's a Mac specific problem, on what kind of machine did you test the code?
The super call is mandatory in my environment, Babel sends me an error if I omit it.

@calvinmetcalf
Copy link
Contributor

I'm on a mac too so it may have been just due to where I downloaded the stuff from

  for (let i = 0; i < this.toPush.length; i++) {
            //console.log("Try to push "+this.toPush[i]);
            if (!this.push(this.toPush[i])) {
                //this._source.readStop();
                break;
            } else {
                //console.log(this.toPush[i]+" pushed");
            }
        }

might have slightly different semantics then

for (let thing of this.toPush) {
          this.push(thing);
        }

in certain circumstances.

the example had 2 calls to super, I removed one

are you running this in a browser or node? and what version of node if it's in node

@mauceri
Copy link

mauceri commented Jun 12, 2017

Yes, I have that file from so long I forgot when I cleant it and I should have thought i'm not alone on earth...
I'm on node, frankly I do not think that loop changes anything at all because the flush is called at the end of the file as expected.

> node --version
v8.0.0

Do you know a way to bypass this use of _flush and test eof inside _transform or _read of a readable stream? I feel like we are not going to solve this problem any time soon and I would like to use stream.

Thank you very much for you help, and forgive my tone, you have excited my susceptibility :(

@calvinmetcalf
Copy link
Contributor

on node 8 you can use _final instead of _flush which is similar but the writable side will not close until done in it

@mauceri
Copy link

mauceri commented Jun 12, 2017

Thanks Calvin, you are great! If it happens you come to Paris I invite you to a good restaurant :)
Have a very nice day, I'll have a good sleep tonight...

@rzade
Copy link

rzade commented Feb 7, 2020

just delete doc.end();

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants