Skip to content

Conversation

@ronag
Copy link
Member

@ronag ronag commented Jul 30, 2020

Provide a hint for the server to keep connection alive until it is
considered no longer re-usable by client. This reduces the risk of
a race where the client sends a request and the server closes it
before receiving it.

I don't think Node core uses this... but I guess it still could be useful.

@ronag ronag requested a review from mcollina July 30, 2020 08:22
@ronag
Copy link
Member Author

ronag commented Jul 30, 2020

@szmarczak

Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

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

Is this header understood by servers at all? I fear we are transferring bytes for no added benefits.. we'll lose some req/sec if this is not needed. Maybe behind an option?

(+1 with throwing anyway)

@ronag
Copy link
Member Author

ronag commented Jul 30, 2020

Is this header understood by servers at all?

I have no idea. I might be interested in implementing it for Node core.

Maybe behind an option?

Unsure if the perf impact is even measurable. Wouldn't mind to put it behind an option if this is a concern. Suggestion for option name?

@ronag
Copy link
Member Author

ronag commented Jul 30, 2020

Removing the send of header, keeping throw, opening issue in Node core to get feedback.

@mcollina
Copy link
Member

I have no idea. I might be interested in implementing it for Node core.

From looking at nginx docs, it seems an header that is send from the server to the client.

From http://nginx.org/en/docs/http/ngx_http_core_module.html

The first parameter sets a timeout during which a keep-alive client connection will stay open on the server side. The zero value disables keep-alive client connections. The optional second parameter sets a value in the “Keep-Alive: timeout=time” response header field. Two parameters may differ.

The “Keep-Alive: timeout=time” header field is recognized by Mozilla and Konqueror. MSIE closes keep-alive connections by itself in about 60 seconds.

@ronag
Copy link
Member Author

ronag commented Jul 30, 2020

@mcollina Thanks! Let's implement it on our side then and adjust idleTimeout accordingly.

@ronag ronag changed the title fix: send keep-alive timeout fix: throw on keep-alive header Jul 30, 2020
@ronag ronag merged commit be9fc08 into master Jul 30, 2020
@ronag ronag deleted the keep-alive branch July 30, 2020 09:05
ronag added a commit that referenced this pull request Jul 30, 2020
@szmarczak
Copy link
Member

👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants