Do not explicitly set Connection-Header. #979
Conversation
If that's the case, are sure sure we know exactly what the repercussions are? Would it be possible to have an option instead? Like |
|
This header only effects the web service and isn't directly linked to SOAP. Maybe it would be better to only set the header to keep-alive, if the forever option is used. In my commit I've commented that line because I've already thought that there might be concerns. |
|
what issue did this solve for you? |
|
I communicate with a Java SOAP-Service and without this change, every second request fails, because the service closed the connection, but the https module got this connection still in its pool after the first successful request (because with #974 pooling is possible, but pooling closed connections does not make any sense). After my change, the connection will not be closed by the counterpart and our pooled connection can be reused correctly, means every request works again. |
I see. Well, being that most connections are keep-alive anyway, I think I'm comfortable merging this knowing that it could be a breaking change. Can you document this in the README? |
|
I've added the change to the readme |
lib/http.js
Outdated
| @@ -43,7 +43,8 @@ HttpClient.prototype.buildRequest = function(rurl, data, exheaders, exoptions) { | |||
| 'Accept': 'text/html,application/xhtml+xml,application/xml,text/xml;q=0.9,*/*;q=0.8', | |||
| 'Accept-Encoding': 'none', | |||
| 'Accept-Charset': 'utf-8', | |||
| 'Connection': 'close', | |||
| // Connection-Header is set in the http or https core module | |||
| // 'Connection': !!exoptions.forever ? 'keep-alive' : 'close', | |||
There was a problem hiding this comment.
is this meant to be commented out?
There was a problem hiding this comment.
See: #979 (comment)
Should I add "next line enables keep-alive connection only for pooled connections" to clearify that?
There was a problem hiding this comment.
I'd replace both lines with 'Connection': !!exoptions.forever ? 'keep-alive' : 'close', and call it good.
lib/http.js
Outdated
| @@ -43,7 +43,8 @@ HttpClient.prototype.buildRequest = function(rurl, data, exheaders, exoptions) { | |||
| 'Accept': 'text/html,application/xhtml+xml,application/xml,text/xml;q=0.9,*/*;q=0.8', | |||
| 'Accept-Encoding': 'none', | |||
| 'Accept-Charset': 'utf-8', | |||
| 'Connection': 'close', | |||
| // Connection-Header is set in the http or https core module | |||
| // 'Connection': !!exoptions.forever ? 'keep-alive' : 'close', | |||
There was a problem hiding this comment.
I'd replace both lines with 'Connection': !!exoptions.forever ? 'keep-alive' : 'close', and call it good.
|
Alright, I've changed that and now the old default behaviour is restored and the forever option will now pool the keep alive connections. |
|
Thanks @Mik13 ! |
|
Thank you too! |
This commit is a follow up to #974
The Connection-Header should not be explicitly set, because the core modules are handling them (see https://github.com/nodejs/node/blob/master/lib/_http_outgoing.js, everything connected to the
shouldKeepAlivefield).This might be a breaking change, because connections are keep-alive by default (see nodejs/node#10774) as long as there are running requests. The forever parameter, which sets the keep-alive flag for the agent, activates pooling (see https://github.com/nodejs/node/blob/2043944a4c62de60e7a04e8c914b565114c4b11b/lib/_http_agent.js#L78-L103), which offers a great performance gain on top.
I did not find a possibility to test this in node, because I've only got problems when connecting to SOAP-Servers written in another language, where every second request fails.