cleanup half closed sockets in connection cache

[Frank Meier]

hi

i've experienced the problem, that curl is not aware of connections that are being closed from the peer. So if the Webserver closes the TCP connection after the keepalive timeout (some secconds), the socket will remain in close_wait state, until the next curl request ist issued. Of course I understand the reason, since my application is already doing something else when the connection closes, there is just no code that calls close() on that socket.

I now liked to change that behavior, and didn't find a nice solution. First I wanted to remeber the sockets and then close it if I see that the socket has been closed by the peer. but this would bypass libcurl, and imho would be a hack.
After further searching I've found a posting in this mailing list, dealing with a similiar problem:
http://curl.haxx.se/mail/lib-2009-03/0009.html
the patch included in this post removed all connections in close_wait before a new request, but still there will be at least one (the last) connection, that will remain in close_wait state "forever".

I now would propose a cleanup fuction which goes throuh the connection cache an removes and closes all connections, which are not usable anymore. Based on the code in ConnectionExists() in url.c I came up with something like the following:

/*
* This function iterates through a given connection cache, and closes
* all sockets which are not usable anymore (eg. in close_wait state)
*/
void Curl_close_dead_connections(struct conncache *connc)
{
long i;
struct connectdata *check;

for(i=0; i< connc->num; i++) {
bool match = FALSE;
size_t pipeLen = 0;

check = connc->connects[i];
if(!check)
/* NULL pointer means not filled-in entry */
continue;

pipeLen = check->send_pipe->size + check->recv_pipe->size;

if(check->connectindex == -1) {
check->connectindex = i; /* Set this appropriately since it might have
been set to -1 when the easy was removed
from the multi */
}

if(!pipeLen && !check->inuse) {
/* The check for a dead socket makes sense only if there are no
handles in pipeline and the connection isn't already marked in
use */
bool dead = SocketIsDead(check->sock[FIRSTSOCKET]);
if(dead) {
Curl_disconnect(check); /* disconnect resources */
connc->connects[i]=NULL; /* nothing here */
}
}
}
}


Did I miss something to deal with this problem, or are there other ideas? feedback is appreciated.

cheers, Frank
-------------------------------------------------------------------
List admin: http://cool.haxx.se/list/listinfo/curl-library
Etiquette: http://curl.haxx.se/mail/etiquette.html

[Daniel Stenberg]

Post by Frank Meier
I now liked to change that behavior, and didn't find a nice solution. First
I wanted to remeber the sockets and then close it if I see that the socket
has been closed by the peer. but this would bypass libcurl, and imho would
be a hack. After further searching I've found a posting in this mailing
http://curl.haxx.se/mail/lib-2009-03/0009.html the patch included in this
post removed all connections in close_wait before a new request

And that patch is merged into the main code since about that time.

Post by Frank Meier
but still there will be at least one (the last) connection, that will remain
in close_wait state "forever".

So you're actually thinking it is worth adding an entirely new function and
code in your app to call that function with some interval just to make sure
there's a single socket being closed correctly?

I personally do not. I would rather prefer to see that scanning to be done
possibly on some other event and not only when adding a new connection, so
that the possibility that it gets discovered and closed increases but without
adding anything new to the API and without doing the check too often.

[Frank Meier]

-----Original Message-----
From: Daniel Stenberg [mailto:***@haxx.se]
Sent: Dienstag, 13. Oktober 2009 05:33
To: libcurl development
Subject: Re: cleanup half closed sockets in connection cache

Post by Frank Meier
I now liked to change that behavior, and didn't find a nice solution. First
I wanted to remeber the sockets and then close it if I see that the socket
has been closed by the peer. but this would bypass libcurl, and imho would
be a hack. After further searching I've found a posting in this mailing
http://curl.haxx.se/mail/lib-2009-03/0009.html the patch included in this
post removed all connections in close_wait before a new request

And that patch is merged into the main code since about that time.

Post by Frank Meier
but still there will be at least one (the last) connection, that will remain
in close_wait state "forever".

So you're actually thinking it is worth adding an entirely new function and
code in your app to call that function with some interval just to make sure
there's a single socket being closed correctly?

I personally do not. I would rather prefer to see that scanning to be done
possibly on some other event and not only when adding a new connection, so
that the possibility that it gets discovered and closed increases but without
adding anything new to the API and without doing the check too often.
--
/ daniel.haxx.se

Post by Frank Meier
So you're actually thinking it is worth adding an entirely new function and
code in your app to call that function with some interval just to make sure
there's a single socket being closed correctly?

I think there should be a possibility to handle this problem, even it it not a
big one. Btw in our case we have an application with a lot of handler child processes, which leads
to a lot of half open connections in the system (seen with netstat for example).
Nevertheless I see your point, having an extra API call to handle this seems to
be a not really generic approach.

Another idea came to my mind, if it might be possible to use the curl_multi_socket_action() call
to tell libcurl that something has happened on a socket under its control, and then lib curl
checks the state of the socket and closes it if its half open. I tried this out, with no
effect: I suppervised the socket in my application (poll) and then called curl_multi_socket_action().
I haven't had time to go any further in this direction (check libcurl code etc) but maybe this
would be possible with a small modification. What do you think of that?

cheers Franky



-------------------------------------------------------------------
List admin: http://cool.haxx.se/list/listinfo/curl-library
Etiquette: http://curl.haxx.se/mail/etiquette.html

[Daniel Stenberg]

Post by Frank Meier
I think there should be a possibility to handle this problem, even it it not
a big one. Btw in our case we have an application with a lot of handler
child processes, which leads to a lot of half open connections in the system
(seen with netstat for example).

Right, and I understand your wish to adress this issue. I'm just not sure how
it is best dealt with.

Post by Frank Meier
Nevertheless I see your point, having an extra API call to handle this seems
to be a not really generic approach.

While it would work, it would be a bit clumpsy (since you'd have to call the
function on chance on some particular interval) and it would leave all exiting
apps to not get the effects until modified.

What about running the thing every time a transfer completes? Or perhaps even
we could do it at completion time and when a handle is removed from the multi
handle.

Post by Frank Meier
Another idea came to my mind, if it might be possible to use the
curl_multi_socket_action() call to tell libcurl that something has happened
on a socket under its control, and then lib curl checks the state of the
socket and closes it if its half open.

That would have to use a new bitmask bit then, as curl_multi_socket_action()
would in some applications be used very often and it would be critical to keep
it very fast in applications using very very many connections. And a new bit
there would basically be as bad as a new function. Besides, there can be idle
connections on the easy interface too so we should preferably make a fix that
could improve things indepdently of what interface that is used.

[Daniel Stenberg]

Post by Daniel Stenberg
What about running the thing every time a transfer completes? Or perhaps
even we could do it at completion time and when a handle is removed from the
multi handle.

I just wanted to check if you've considered this approach any further? It
would be a way to provide the functionality but without changing API/ABI...

[Frank Meier]

sorry I didn't reply, I lost track of this thread somehow :-/

Post by Daniel Stenberg
What about running the thing every time a transfer completes? Or perhaps
even we could do it at completion time and when a handle is removed from the
multi handle.

that would not be of much help, because (at least in our case) the servers we are taking to have keepalive enabled, so the socket get's into close_wait after serveral seconds, when the process is already doing something else. And If there is a new request using curl_perform all the half-open connections in the connection cache will be closed anyway (since patch http://curl.haxx.se/mail/lib-2009-03/0009.html). So the Problem exists when a lot of backend connections are established, and later (few seconds) they are being closed by the peer. At that point in time I should be able to detect this and to close the connection from my side.

Generally in an application where I had to handle a lot of sockets, the (IMHO) right approach would be to put all this sockets (fdset) in a select, and depending on which one something is happending (close, accept, data to read) you can handle it appropreatly. In the meantime the application can suspend in the select, or do something else. So its possible to react "~immediatly" to actions on sockets, and otherwise the application does only run if necessary. Now with libCurl as it is now I don't see this kind of possibility (without extending the API, I'm afraid). Even if It would be possible to get an fdset of all the sockets in the connection cache, maybe by curl_easy_getinfo(), I'd need a function like curl_handle_socket() to handle the socket action (close read etc), just closing the so
cket, that is und curl's control wouldn't be nice I think.

Another approch I see, to cleanup the connection cache with each call to curl_multi_perform. Since the connection cache is attached to the multihandle, and curl_multi_perform does not block, it could be used as a cleanup function that has to be called from time to time (not so nice). An improvement could be only to cleanup the cache if no running easy_handles are attached to the multi_handle. In the easy_interface something similiar could be done if URL is set to NULL, curl_easy_perform() only cleans up the cache (like it does now if a URL is given).

As I see the extension of the API is not an option for you, so I would suggest to consider at least the idea with the cleanup function in curl_xxx_perform. So even it is not the nicest way to go, there would be a possibility to cleanup all half-closed connections, not driven by an event, but better than nothing.

If you like one of these ideas I could propose a patch

cheers, Frank
-------------------------------------------------------------------
List admin: http://cool.haxx.se/list/listinfo/curl-library
Etiquette: http://curl.haxx.se/mail/etiquette.html

[Daniel Stenberg]

Post by Frank Meier

Post by Daniel Stenberg
What about running the thing every time a transfer completes? Or perhaps
even we could do it at completion time and when a handle is removed from
the multi handle.

that would not be of much help, because (at least in our case) the servers
we are taking to have keepalive enabled, so the socket get's into close_wait
after serveral seconds, when the process is already doing something else.

I see. But I was thinking more of a case there you use N connections and they
all complete with Y seconds interval. Then it would make sense to check the
connection cache when each transfer ends, as the the previous transfer might
be possible to close correctly. This is something we don't do today since the
current logic is more limited.

Post by Frank Meier
So the Problem exists when a lot of backend connections are established, and
later (few seconds) they are being closed by the peer. At that point in time
I should be able to detect this and to close the connection from my side.

Right, when N transfers complete almost at once we end up with all of them
going dead if you don't do anything else within Z seconds...

Post by Frank Meier
Generally in an application where I had to handle a lot of sockets, the
(IMHO) right approach would be to put all this sockets (fdset) in a select,
and depending on which one something is happending (close, accept, data to
read) you can handle it appropreatly.

Yes, that is the right way to deal with many sockets but in libcurls case it
knows of many more sockets than what you right now deal with. Adding them too
to the select() would for most use-cases just be an extra set of handles that
only makes select() slower.

Post by Frank Meier
In the meantime the application can suspend in the select, or do something
else. So its possible to react "~immediatly" to actions on sockets, and
otherwise the application does only run if necessary. Now with libCurl as it
is now I don't see this kind of possibility (without extending the API, I'm
afraid).

Right, but then also most applications don't care about the
sockets/connections that aren't currently in use. And I think rightfully so.
The connection cache can potentially be a lot of connections.

Post by Frank Meier
Even if It would be possible to get an fdset of all the sockets in the
connection cache, maybe by curl_easy_getinfo(), I'd need a function like
curl_handle_socket() to handle the socket action (close read etc), just
closing the socket, that is und curl's control wouldn't be nice I think.

Right, if you'd close them libcurl would not consider that nice.

Post by Frank Meier
Another approch I see, to cleanup the connection cache with each call to
curl_multi_perform. Since the connection cache is attached to the
multihandle, and curl_multi_perform does not block

Right, but it's a potentially rather expensive operation so we shouldn't it on
_every_ call. We'd need some cleverer approach.

Perhaps we could make it do that check every N seconds, where N would be a
conservatively high value (at least 60 seconds, possibly even 300 or so) to
start with and we add a new curl_multi/easy_setopt() option to allow it to get
set to something smaller.

Post by Frank Meier
it could be used as a cleanup function that has to be called from time to
time (not so nice).

Well, in your use-case when you use N connections for a while and then stop
using them all at once, there's really no other way than somehow call libcurl
again at a later point only for the purpose of closing down "dead"
connections. The question is then basically only what function to call. I
can't even think of a way to help the app to know for how long to wait or how
often to do the maintainance call. We just don't know those things.

Post by Frank Meier
An improvement could be only to cleanup the cache if no running easy_handles
are attached to the multi_handle.

Perhaps, but I think having it based on time is slightly better since it'll
then also cover the case when you do 10 transfers and 9 of them stop at once
and the last one takes another hour to complete.

Post by Frank Meier
In the easy_interface something similiar could be done if URL is set to
NULL, curl_easy_perform() only cleans up the cache (like it does now if a
URL is given).

Well, that would make it a really odd exception to how curl_easy_perform() is
used today so I'm not sure I'm that comfortable with such a solution. After
all, if you've done your curl_easy_perform() you can force a closure of
everything by simply closing the easy handle.

Post by Frank Meier
As I see the extension of the API is not an option for you

It _is_ an option. It's just an option I go very far to avoid so I hope
throwing ideas around and discussing around them might make us come up with a
way we all like!

[Frank Meier]

Post by Frank Meier

Post by Daniel Stenberg
What about running the thing every time a transfer completes? Or perhaps
even we could do it at completion time and when a handle is removed from
the multi handle.

that would not be of much help, because (at least in our case) the servers
we are taking to have keepalive enabled, so the socket get's into close_wait
after serveral seconds, when the process is already doing something else.
I see. But I was thinking more of a case there you use N connections and they
all complete with Y seconds interval. Then it would make sense to check the
connection cache when each transfer ends, as the the previous transfer might
be possible to close correctly. This is something we don't do today since the
current logic is more limited.

I have the feeling it does not make a big difference, because now we cleanup the cache
at the beginning of of each request. If we would do it after (a few 100ms
later), there is the chance that some more connections are being cleaned up.
But on the other hand we had to run through the cache once more only to search for dead
connections, and it does not solve the problem. So it's a trade off.

Post by Frank Meier
Perhaps we could make it do that check every N seconds, where N would be a
conservatively high value (at least 60 seconds, possibly even 300 or so) to
start with and we add a new curl_multi/easy_setopt() option to allow it to get
set to something smaller.

I quite like that, but It would only help with the multi interface. right?
And the timer could be reseted on each curl_easy_add(), because then a new
transfer starts anyway, and if the timer has expired when calling
curl_multi_perform() the cleanup is done.

Post by Frank Meier
Perhaps, but I think having it based on time is slightly better since it'll
then also cover the case when you do 10 transfers and 9 of them stop at once
and the last one takes another hour to complete.

Agreed

Post by Frank Meier
Well, that would make it a really odd exception to how curl_easy_perform() is
used today so I'm not sure I'm that comfortable with such a solution. After
all, if you've done your curl_easy_perform() you can force a closure of
everything by simply closing the easy handle.

But closing everything is not the goal, only the dead connections. In my opinion it
is not an odd behavior, because when an URL is given curl_easy_perform() does the cleanup
and the performs the request, if no URL is given, it does only the cleanup.

I would prefer the first approach with the timeout in curl_multi_perform(), although
the people using the easy interface have no possibility to cleanup the connection cache.

cheers, Frank