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

http: don't write empty data on req/res end() #41116

Merged
merged 1 commit into from
Dec 10, 2021

Conversation

santigimeno
Copy link
Member

When calling OutgoingMessage.end() with empty data argument, avoid
writing to the socket unless there's still pending data to be sent.

Fixes: #41062

@nodejs-github-bot nodejs-github-bot added http Issues or PRs related to the http subsystem. needs-ci PRs that need a full CI run. labels Dec 8, 2021
@nodejs-github-bot

This comment has been minimized.

this._send('', 'latin1', finish);
} else {
process.nextTick(finish);
Copy link
Member

@lpinca lpinca Dec 8, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The problem I see here is that the 'finish' event might be emitted before buffered data is flushed and similarly the outgoingMessage.end() callback might be called before previous outgoingMessage.write() callbacks.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I understand, but I think at this point there should not buffered data and a consecutive call to end() should not reach here, or am I missing smthg? Thanks

Copy link
Member

@lpinca lpinca Dec 8, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What do you think about adding a check for socket.writableLength to the above else if branch?

...
} else if (!this._headerSent || this.outputSize || chunk || this.socket?.writableLength) {
  this._send('', 'latin1', finish);
} else {
...

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That sounds good. We might even use OutgoingMessage.writableLength directly. Thanks!

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@lpinca pushed the changes

Copy link
Member

@lpinca lpinca Dec 8, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think at this point there should not buffered data and a consecutive call to end() should not reach here

I think you are right because the transfer encoding would be chunked but I'm not sure if there are cases where that path can be taken anyway. For example in #41062 the user specifies the Content-Length header so the chunked encoding is not used.

When calling OutgoingMessage.end() with empty data argument, avoid
writing to the socket unless there's still pending data to be sent.

Fixes: nodejs#41062
@nodejs-github-bot

This comment has been minimized.

@nodejs-github-bot

This comment has been minimized.

@nodejs-github-bot

This comment has been minimized.

@nodejs-github-bot
Copy link
Collaborator

@lpinca lpinca added the commit-queue Add this label to land a pull request using GitHub Actions. label Dec 10, 2021
@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Dec 10, 2021
@nodejs-github-bot nodejs-github-bot merged commit ef7a686 into nodejs:master Dec 10, 2021
@nodejs-github-bot
Copy link
Collaborator

Landed in ef7a686

danielleadams pushed a commit that referenced this pull request Dec 13, 2021
When calling OutgoingMessage.end() with empty data argument, avoid
writing to the socket unless there's still pending data to be sent.

Fixes: #41062

PR-URL: #41116
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Gerhard Stöbich <deb2001-github@yahoo.de>
Reviewed-By: James M Snell <jasnell@gmail.com>
danielleadams pushed a commit that referenced this pull request Dec 14, 2021
When calling OutgoingMessage.end() with empty data argument, avoid
writing to the socket unless there's still pending data to be sent.

Fixes: #41062

PR-URL: #41116
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Gerhard Stöbich <deb2001-github@yahoo.de>
Reviewed-By: James M Snell <jasnell@gmail.com>
@saberph
Copy link

saberph commented Jan 19, 2022

Hi,

Do you know when it will be pushed to v14 LTS branch? We are affected by this too.

Thank you =)

danielleadams pushed a commit that referenced this pull request Jan 31, 2022
When calling OutgoingMessage.end() with empty data argument, avoid
writing to the socket unless there's still pending data to be sent.

Fixes: #41062

PR-URL: #41116
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Gerhard Stöbich <deb2001-github@yahoo.de>
Reviewed-By: James M Snell <jasnell@gmail.com>
danielleadams pushed a commit that referenced this pull request Jan 31, 2022
When calling OutgoingMessage.end() with empty data argument, avoid
writing to the socket unless there's still pending data to be sent.

Fixes: #41062

PR-URL: #41116
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Gerhard Stöbich <deb2001-github@yahoo.de>
Reviewed-By: James M Snell <jasnell@gmail.com>
Linkgoron pushed a commit to Linkgoron/node that referenced this pull request Jan 31, 2022
When calling OutgoingMessage.end() with empty data argument, avoid
writing to the socket unless there's still pending data to be sent.

Fixes: nodejs#41062

PR-URL: nodejs#41116
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Gerhard Stöbich <deb2001-github@yahoo.de>
Reviewed-By: James M Snell <jasnell@gmail.com>
danielleadams pushed a commit that referenced this pull request Feb 1, 2022
When calling OutgoingMessage.end() with empty data argument, avoid
writing to the socket unless there's still pending data to be sent.

Fixes: #41062

PR-URL: #41116
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Gerhard Stöbich <deb2001-github@yahoo.de>
Reviewed-By: James M Snell <jasnell@gmail.com>
@danielleadams danielleadams mentioned this pull request Feb 1, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
http Issues or PRs related to the http subsystem. needs-ci PRs that need a full CI run.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

http requests with socketPath lead to EPIPE error
6 participants