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

HTTPClient and keep alive #448

Closed
gittywithexcitement opened this Issue Dec 27, 2013 · 2 comments

Comments

Projects
None yet
2 participants
@gittywithexcitement
Contributor

gittywithexcitement commented Dec 27, 2013

I'm using vibe.d's requestHTTP method to do stuff with an elasticsearch server. By default, vibe.d sends an HTTP 1.1 request with Connection: keep-alive header. The elasticsearch server returns a response:

HTTP client response:
---------------------
HTTP/1.1 201
Content-Type: application/json; charset=UTF-8
Content-Length: 98

Because the response does not contain a keep alive header with a timeout value, HTTPClient assumes that the connection will be closed by the server.
m_keepAliveLimit is not set
https://github.com/rejectedsoftware/vibe.d/blob/master/source/vibe/http/client.d#L531-L543
so disconnect is called before transmission of the next request
https://github.com/rejectedsoftware/vibe.d/blob/master/source/vibe/http/client.d#L302-L304
However, elasticsearch thinks that the connection is persistent, so it leaves the connection open for a while (the timeout is around 1-2 minutes). This is not a big deal for one request, but when doing many requests per second, I run out of ephemeral ports. My system settings allow up to 28,000 ephemeral ports. I turn on my program that talks to elasticsearch through vibe.d and those 28,000 ports are used up in less than a minute and then I get errors because I can't bind to any more ports.

A single line of netstat output looks like (elasticsearch is on port 9200): tcp 0 0 127.0.0.1:38836 127.0.0.1:9200 TIME_WAIT as you can see elasticsearch is keeping the other end of the socket open until it times out. If I run my program I quickly see 28,000 of lines like that, and then run out of ephemeral ports.

You can test this yourself with the following test program.
https://gist.github.com/gittywithexcitement/8141734 and run elasticsearch or write your own server that doesn't include the keep alive header.
Before and after running the gist, execute:
netstat -an | grep -e tcp wc -l which will count the number of TCP connections. Notice how the count goes up by 1000 (the gist's loop variable). My local port range is 28,644 ports (cat /proc/sys/net/ipv4/ip_local_port_range) So if I set the loop variable to 30,000 the program fails with
std.exception.ErrnoException@../vibe.d/source/vibe/core/drivers/libevent2.d(249): Failed to bind socket. (Address already in use)

Based on my reading of http://www.w3.org/Protocols/rfc2616/rfc2616-sec8.html#sec8.1.2.1 it seems to me that HTTPClient should assume the connection will stay open when an HTTP 1.1 response is received and there is no connection-token close.

OS: linux Ubuntu 13.10
vibe.d version: master:e617354b655d654aa08dab93ae96517dd196d2f7
DMD64 D Compiler v2.064

@gittywithexcitement

This comment has been minimized.

Show comment
Hide comment
@gittywithexcitement

gittywithexcitement Dec 28, 2013

Contributor

The immediate issue has been solved with the merge 23e5739 Thanks!
However, you had a couple of TODO items you mentioned in the pull request, #450 (comment)

I think the best would be a 2 step solution:

  1. lower the 2.seconds by default and make it configurable, as well as the default timeout
  2. implement automatic request retry if sending the request header on an existing keep-alive connection has failed

The only issue with 2 is in cases where a request body was written, the user supplied request callback would have to be invoked twice if the error occurs late (maybe when the response header is being read). This might cause some unexpected things if the callback has side effects.

So I'll leave this issue open as a reminder for those items.

Contributor

gittywithexcitement commented Dec 28, 2013

The immediate issue has been solved with the merge 23e5739 Thanks!
However, you had a couple of TODO items you mentioned in the pull request, #450 (comment)

I think the best would be a 2 step solution:

  1. lower the 2.seconds by default and make it configurable, as well as the default timeout
  2. implement automatic request retry if sending the request header on an existing keep-alive connection has failed

The only issue with 2 is in cases where a request body was written, the user supplied request callback would have to be invoked twice if the error occurs late (maybe when the response header is being read). This might cause some unexpected things if the callback has side effects.

So I'll leave this issue open as a reminder for those items.

@s-ludwig

This comment has been minimized.

Show comment
Hide comment
@s-ludwig

s-ludwig Jan 10, 2016

Member
  1. lower the 2.seconds by default and make it configurable, as well as the default timeout

Fixed by #756

  1. implement automatic request retry if sending the request header on an existing keep-alive connection has failed

Fixed in 560290a.

Member

s-ludwig commented Jan 10, 2016

  1. lower the 2.seconds by default and make it configurable, as well as the default timeout

Fixed by #756

  1. implement automatic request retry if sending the request header on an existing keep-alive connection has failed

Fixed in 560290a.

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