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: send connection: close on client error #26467

Closed

Conversation

Projects
7 participants
@yannh
Copy link
Contributor

commented Mar 6, 2019

HTTP/1.1 mandates connections which do not support keep-alive and
close the connection send the connection: close header, see
https://www.w3.org/Protocols/rfc2616/rfc2616-sec14.html#sec14.10

This page also provides more information:
https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Connection

I understand that HTTP/1.1 defaults to keep-alive - and that the
Connection: close header is required when closing a connection.

This adds the Connection: close header in the 400 and 414
responses sent on client errors.

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines
http: send connection: close when closing conn
HTTP/1.1 mandates connections which do not support keep-alive and
close the connection send the connection: close header, see
https://www.w3.org/Protocols/rfc2616/rfc2616-sec14.html#sec14.10

This page also provides more information:
https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Connection

I understand that HTTP/1.1 defaults to keep-alive - and that the
Connection: close header is required when closing a connection.

This adds the Connection: close header in the 400 and 414
responses sent on client errors.
@BridgeAR

This comment has been minimized.

@BridgeAR BridgeAR requested a review from lpinca Mar 6, 2019

@lpinca

lpinca approved these changes Mar 7, 2019

BridgeAR added a commit to BridgeAR/node that referenced this pull request Mar 8, 2019

http: send connection: close when closing conn
HTTP/1.1 mandates connections which do not support keep-alive and
close the connection send the connection: close header, see
https://www.w3.org/Protocols/rfc2616/rfc2616-sec14.html#sec14.10

This page also provides more information:
https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Connection

I understand that HTTP/1.1 defaults to keep-alive - and that the
Connection: close header is required when closing a connection.

This adds the Connection: close header in the 400 and 414
responses sent on client errors.

PR-URL: nodejs#26467
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
@BridgeAR

This comment has been minimized.

Copy link
Member

commented Mar 8, 2019

Landed in c957b05

@yannh congratulations to your first commit to Node.js! 🎉

@BridgeAR

This comment has been minimized.

Copy link
Member

commented Mar 12, 2019

This relies on a semver-major commit. I therefore added the do not land labels.

@johanneswuerbach

This comment has been minimized.

Copy link

commented Mar 12, 2019

@BridgeAR could you elaborate why this is semver-major? I somehow understand that this is a change in behaviour, but on the other hand I think its mainly a bug fix so nodejs is by default adhering to the http 1.1 spec.

@lpinca

This comment has been minimized.

Copy link
Member

commented Mar 13, 2019

@johanneswuerbach because it depends on #25605 which was tagged as semver-major.

@johanneswuerbach

This comment has been minimized.

Copy link

commented Mar 13, 2019

Thanks @lpinca, would you accept a PR targeting 10/11, which only changes this for 400 responses? I’m especially interested in getting this backported into 10 LTS.

Is this something, which node does and if yes, do you have some pointers to how its done? Just cherry-pick parts of this?

@lpinca

This comment has been minimized.

Copy link
Member

commented Mar 13, 2019

Yes, I think we can have a partial backport.

Just cherry-pick parts of this?

Yes.

johanneswuerbach added a commit to johanneswuerbach/node that referenced this pull request Mar 13, 2019

http: send connection: close when closing conn
HTTP/1.1 mandates connections which do not support keep-alive and
close the connection send the connection: close header, see
https://www.w3.org/Protocols/rfc2616/rfc2616-sec14.html#sec14.10

This page also provides more information:
https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Connection

I understand that HTTP/1.1 defaults to keep-alive - and that the
Connection: close header is required when closing a connection.

This adds the Connection: close header in the 400 responses
sent on client errors.

PR-URL: nodejs#26467
@johanneswuerbach

This comment has been minimized.

Copy link

commented Mar 13, 2019

Created a backport for node 10 #26627 / node 11 #26646

johanneswuerbach added a commit to johanneswuerbach/node that referenced this pull request Mar 13, 2019

http: send connection: close when closing conn
HTTP/1.1 mandates connections which do not support keep-alive and
close the connection send the connection: close header, see
https://www.w3.org/Protocols/rfc2616/rfc2616-sec14.html#sec14.10

This page also provides more information:
https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Connection

I understand that HTTP/1.1 defaults to keep-alive - and that the
Connection: close header is required when closing a connection.

This adds the Connection: close header in the 400 responses
sent on client errors.

PR-URL: nodejs#26467

BridgeAR added a commit that referenced this pull request Mar 14, 2019

http: send connection: close when closing conn
HTTP/1.1 mandates connections which do not support keep-alive and
close the connection send the connection: close header, see
https://www.w3.org/Protocols/rfc2616/rfc2616-sec14.html#sec14.10

This page also provides more information:
https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Connection

I understand that HTTP/1.1 defaults to keep-alive - and that the
Connection: close header is required when closing a connection.

This adds the Connection: close header in the 400 responses
sent on client errors.

PR-URL: #26467

BridgeAR added a commit that referenced this pull request Mar 14, 2019

http: send connection: close when closing conn
HTTP/1.1 mandates connections which do not support keep-alive and
close the connection send the connection: close header, see
https://www.w3.org/Protocols/rfc2616/rfc2616-sec14.html#sec14.10

This page also provides more information:
https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Connection

I understand that HTTP/1.1 defaults to keep-alive - and that the
Connection: close header is required when closing a connection.

This adds the Connection: close header in the 400 responses
sent on client errors.

PR-URL: #26467
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>

BridgeAR added a commit that referenced this pull request Mar 14, 2019

http: send connection: close when closing conn
HTTP/1.1 mandates connections which do not support keep-alive and
close the connection send the connection: close header, see
https://www.w3.org/Protocols/rfc2616/rfc2616-sec14.html#sec14.10

This page also provides more information:
https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Connection

I understand that HTTP/1.1 defaults to keep-alive - and that the
Connection: close header is required when closing a connection.

This adds the Connection: close header in the 400 responses
sent on client errors.

PR-URL: #26467
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>

BridgeAR added a commit to BridgeAR/node that referenced this pull request Mar 14, 2019

http: send connection: close when closing conn
HTTP/1.1 mandates connections which do not support keep-alive and
close the connection send the connection: close header, see
https://www.w3.org/Protocols/rfc2616/rfc2616-sec14.html#sec14.10

This page also provides more information:
https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Connection

I understand that HTTP/1.1 defaults to keep-alive - and that the
Connection: close header is required when closing a connection.

This adds the Connection: close header in the 400 responses
sent on client errors.

PR-URL: nodejs#26467
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>

BridgeAR added a commit that referenced this pull request Mar 14, 2019

http: send connection: close when closing conn
HTTP/1.1 mandates connections which do not support keep-alive and
close the connection send the connection: close header, see
https://www.w3.org/Protocols/rfc2616/rfc2616-sec14.html#sec14.10

This page also provides more information:
https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Connection

I understand that HTTP/1.1 defaults to keep-alive - and that the
Connection: close header is required when closing a connection.

This adds the Connection: close header in the 400 responses
sent on client errors.

PR-URL: #26467
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>

@targos targos added this to Backported in v11.x Mar 27, 2019

BethGriggs added a commit that referenced this pull request Apr 16, 2019

http: send connection: close when closing conn
HTTP/1.1 mandates connections which do not support keep-alive and
close the connection send the connection: close header, see
https://www.w3.org/Protocols/rfc2616/rfc2616-sec14.html#sec14.10

This page also provides more information:
https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Connection

I understand that HTTP/1.1 defaults to keep-alive - and that the
Connection: close header is required when closing a connection.

This adds the Connection: close header in the 400 responses
sent on client errors.

PR-URL: #26467

BethGriggs added a commit that referenced this pull request Apr 16, 2019

http: send connection: close when closing conn
HTTP/1.1 mandates connections which do not support keep-alive and
close the connection send the connection: close header, see
https://www.w3.org/Protocols/rfc2616/rfc2616-sec14.html#sec14.10

This page also provides more information:
https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Connection

I understand that HTTP/1.1 defaults to keep-alive - and that the
Connection: close header is required when closing a connection.

This adds the Connection: close header in the 400 responses
sent on client errors.

Backport-PR-URL: #26627
PR-URL: #26467
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>

@BethGriggs BethGriggs referenced this pull request May 1, 2019

Merged

v10.16.0 proposal #27514

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.