tim-smart / multipasta

MIT License
2 stars 1 forks source link

Backpressure implementation seems incorrect #30

Open thynson opened 10 months ago

thynson commented 10 months ago

https://github.com/tim-smart/multipasta/blob/bf352fbf3a9fcd07e733068430fcfc00dedbfd15/src/node.ts#L61

According to the document, when Readable.push returns false, no more data should be pushed to it anymore until _read being called.

This requires changing the onFile callback, and, ultimately the Parser itself needs to be able to pause/resume.

Also, src/node.ts and src/web.ts should be exported through package.json#exports since you set type to be "module".

tim-smart commented 10 months ago

When _canWrite is false, writes are suspended until _read is called on the current file stream: https://github.com/tim-smart/multipasta/blob/main/src/node.ts#L123

tim-smart commented 10 months ago

Regarding exports, they are adding during the build. You can see the final package.json on npm here: https://www.npmjs.com/package/multipasta?activeTab=code

thynson commented 10 months ago

When _canWrite is false, writes are suspended until _read is called on the current file stream:

But we need to pause the Parser, otherwise onFile or onField would be still called even if _canWrite is false.

thynson commented 10 months ago

While the ability to pause/resume is not implemented for Parser yet.

tim-smart commented 10 months ago

The parser is fully synchronous, so to pause it you just need to stop writing to it :)

thynson commented 10 months ago

But I'm concerning that even if the write side were paused, the last chunk written to the parser could result in too many events emitted.

And the return value of these this.push is unchecked: https://github.com/tim-smart/multipasta/blob/bf352fbf3a9fcd07e733068430fcfc00dedbfd15/src/node.ts#L53 and https://github.com/tim-smart/multipasta/blob/bf352fbf3a9fcd07e733068430fcfc00dedbfd15/src/node.ts#L58

tim-smart commented 10 months ago

Right, I'm not too worried about that. Node has its own buffer for this - the main thing is that you pause the upstream to propagate the backpressure.