GNOME Bugzilla – Bug 596074
Session leak when aborted before queued message finished
Last modified: 2009-11-22 14:01:25 UTC
Created attachment 143787 [details] Session leak test case with queue-abort sequence When queueing a message on an async session and aborting the session before the message gets a response, the session is leaked. (I referred to this situation in Bug #498509, but now have finally found the time to really study it.) The attached test case shows the problem (with libsoup-2.27.5).
I think it's in soup_socket_disconnect(): idle_connect_callback isn't called upon disconnect, so the connect callback (which unrefs the session) doesn't get invoked. Also sacd isn't freed.
I think it shouldn't be reffing the session that it passes to soup_connection_connect_async, because that's backwards; we don't want the session to stick around because a connection is in progress, we want the connection to be cancelled if the session is disposed(). But just making that change causes proxy-test to crash, so it needs a little work
Created attachment 143802 [details] [review] Proposed patch (against 2.27.5) The attached patch fixes the test at least, by calling idle_connect_result() when disconnecting. I tried to use soup_add_completion() for it, but that didn't seem to work (it never got called, even with the g_main_context_iteration in the test). There are a lot of changes because the sacd had to be added to SoupSocketPrivate. Note that the patch is against 2.27.5, but it applies with some offset to git HEAD.
(In reply to comment #2) > I think it shouldn't be reffing the session that it passes to > soup_connection_connect_async, because that's backwards; we don't want the > session to stick around because a connection is in progress, we want the > connection to be cancelled if the session is disposed(). The following comment in got_connection makes me think it's reasonable to ref the session: /* Even if the connection failed, we run the queue, since * there may have been messages waiting for the connection * count to go down. */
Created attachment 145287 [details] [review] Don't leak session if message is cancelled during socket connection fixes the provided test case and doesn't break any existing test cases. It's tricky to make a good test case for this. (I can't find anything that actually says 1.1.1.1 is guaranteed to have a route pointing to it.) However, some changes for bug 598163 will make it easier to test this reliably.
Attachment 145287 [details] pushed as 3076f99 - Don't leak session if message is cancelled during socket connection fixed in master. will eventually end up in libsoup 2.28.2 in a few weeks