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

docs: clarify writableNeedDrain behavior and usage in streams #56419

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

oussamaBGZ
Copy link

@oussamaBGZ oussamaBGZ commented Dec 31, 2024

This PR updates the documentation for the writableNeedDrain property in the streams module to clarify its behavior and guide developers on how to handle backpressure effectively.

Problem
Inconsistent handling of backpressure can lead to issues like incomplete data transfer.

For example:

  1. Correct Backpressure Handling: The following code correctly handles backpressure by using the return value of writable.write(chunk) and listens for the 'drain' event to resume the readable stream:
image
  1. Incorrect Backpressure Handling: The following code checks writableNeedDrain directly, which leads to incomplete copying (only 80% of the file is copied) because writableNeedDrain isn't updated immediately after each write():
image

@nodejs-github-bot nodejs-github-bot added doc Issues and PRs related to the documentations. stream Issues and PRs related to the stream subsystem. labels Dec 31, 2024
@lpinca
Copy link
Member

lpinca commented Dec 31, 2024

The problem in your example seems to be that the chunk of data is not written when writable.writableNeedDrain is true, so it is simply discarded.

@lpinca lpinca dismissed a stale review December 31, 2024 19:35

Approved too quickly without enough thought

@oussamaBGZ
Copy link
Author

The problem in your example seems to be that the chunk of data is not written when writable.writableNeedDrain is true, so it is simply discarded.

when its discarded the read stream is paused so technically we should not lost any data.
the example doesn't skip chunks intentionally, the problem lies in the timing of when writable.writableNeedDrain is updated.

@lpinca
Copy link
Member

lpinca commented Dec 31, 2024

when its discarded the read stream is paused so technically we should not lost any data

No, you get the chunk, the chunk it not written (else branch in your example), and then the stream is paused. The chunk is lost forever. You should at least write it before pausing the stream.

@oussamaBGZ
Copy link
Author

e chunk it not wri

Ahh ok thanks a lot, i added writable.write(chunk) before the pause func, the outpout file size was exactly the same. It was a bit tricky to notice hahaha maybe we could add a section in the docs to talk about situations like these

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
doc Issues and PRs related to the documentations. stream Issues and PRs related to the stream subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants