GNOME Bugzilla – Bug 732090
gvfsd-http runs out of file descriptors: many sockets in ESTABLISHED or CLOSE_WAIT state
Last modified: 2014-07-29 13:31:40 UTC
Created attachment 278981 [details] Testcase for this bug Dear gvfs developers, since some time (in fact, more than a year, but as until now it wasn't critical I always postponed reporting it), I have noticed a problem with gvfsd-http and GIO. That is, sockets used to retrieve files over the network stay in the ESTABLISHED or CLOSE_WAIT state for ages... and thus the number of FDs is growing and growing. I have a long-running program I wrote (and I know also for Rhythmbox this was an issue), which soon runs out of file descriptors, leading to a failure. To help better seeing the problem, I have written a small program using Gtkmm, which exhibits the issue. It is attached here. To watch it run out of file descriptors, increase the amount of fetches to 1001. I tried digging into GDaemonFile's source, which I think is the client counterpart to gvfsd-http, and I *think* the problem is at least that, upon destruction of the GFile it subtypes, gvfsd-http does not get notified to close the socket. In alternative, closing the socket upon a call to g_file_read_finish(), inside GIO, might be the correct think to do. I posted the same problem on gvfs's ML, and Ross Lagerwall pointed out that: --------------------- This bug has been logged before as gvfs bug 639777 and was tracked down as libsoup bug 695652. If you're having this issue even though you're using a recent version of libsoup, please open a new bug on GNOME Bugzilla against gvfs. ---------------------
This is mostly a libsoup issue, so I shall change the bug product. The problem is that libsoup doesn't notice when a persistent connection is remotely closed by the server and so it remains in the CLOSE_WAIT state until a connection to that server is reopened. This problem is made worse by the fact that due to the gvfs design, it uses a separate SoupSession for each different URL, so in your test case where each URL is different, you end up with tons of CLOSE_WAIT sockets hanging around. I am attaching a copy-pasted libsoup example which will leave a socket in the CLOSE_WAIT state when run against a server with a persistent connection.
Created attachment 279237 [details] libsoup example
Hey Dan, I found the following comment on the Webkit GTK mailing list: """ This is one of the leftover bits of historical lameness in libsoup. It doesn't pay attention to persistent connections when they're idle. So if the server times out a connection and closes it, it will show up in CLOSE_WAIT on the client for a while. Libsoup will only notice it when either you try to connect to that server again, or if it hits the maximum-number-of-open-connections limit. -- Dan """ Any chance this can be fixed? Naively, it shouldn't be that hard with an async session to listen for read-ready signals on idle connections so that it can be detected when the remote server closes the connection. Bug 721064 is probably a duplicate. Thanks!
(In reply to comment #3) > Bug 721064 is probably a duplicate. Well, that was there first, so *this* is the duplicate. But let's bounce it back to gvfs instead, since it looks like gvfs actually is leaking resources; a new GVfsBackendHttp gets created for every http: GFile that is created, but they apparently never get freed. (Or maybe they get deleted after a timeout?) If the backend got freed when gvfs was done with it, then it would free its SopuSession, and so the idle connection would get closed. However, ignoring that, it would be much better anyway if gvfsd-http kept a single global SoupSession (rather than creating a new one for every GVfsBackendHttp), so that it could actually *use* these persistent open connections. Right now, if you open http://example.com/a.txt and then http://example.com/b.txt, gvfsd-http will open a connection to example.com, fetch a.txt, leave the connection open, then open a second connection to example.com to fetch b.txt, even though the first one was still open. Using a single global session would also guarantee that, no matter how many requests you made, there would never be more than SoupSession:max-conns connections open.
(In reply to comment #4) > (In reply to comment #3) > > Bug 721064 is probably a duplicate. > > Well, that was there first, so *this* is the duplicate. True :-) > > But let's bounce it back to gvfs instead, since it looks like gvfs actually is > leaking resources; a new GVfsBackendHttp gets created for every http: GFile > that is created, but they apparently never get freed. (Or maybe they get > deleted after a timeout?) > > If the backend got freed when gvfs was done with it, then it would free its > SopuSession, and so the idle connection would get closed. > > However, ignoring that, it would be much better anyway if gvfsd-http kept a > single global SoupSession (rather than creating a new one for every > GVfsBackendHttp), so that it could actually *use* these persistent open > connections. Right now, if you open http://example.com/a.txt and then > http://example.com/b.txt, gvfsd-http will open a connection to example.com, > fetch a.txt, leave the connection open, then open a second connection to > example.com to fetch b.txt, even though the first one was still open. > > Using a single global session would also guarantee that, no matter how many > requests you made, there would never be more than SoupSession:max-conns > connections open. Yeah, I agree that both of these gvfs issues need fixing. And it's probably time to get rid of the sync and async SoupSessions. I will reassign this back to gvfs. However, I still think it would be nice if libsoup could close CLOSE_WAIT connections promptly, but I'll leave that to bug 721064. Thanks
Created attachment 279718 [details] [review] http: Remove use of SoupSessionSync/SoupSessionAsync Since libsoup 2.42.0, SoupSessionSync/SoupSessionAsync are deprecated, and replaced by direct use of SoupSession as described on https://developer.gnome.org/libsoup/stable/libsoup-session-porting.html This commit removes use of SoupSessionSync/SoupSessionAsync and adjusts the code according to the advice in the doc above: - we only need one SoupSession instance as sync/async calls are made depending on the SoupSession method we use, not depending on the instance type - SoupSession already comes with a SoupProxyResolverDefault, we don't need to add it ourselves - SoupSession already comes with a SoupContentDecoder, we don't need to add it ourselves - SoupSession:use-thread-context is now unused and always set to TRUE, so we don't need to change it To prevent any changes in behavior, we set ssl-strict to FALSE. (Patch updated by Ross Lagerwall) https://bugzilla.gnome.org/show_bug.cgi?id=708306
Created attachment 279719 [details] [review] http: Use a global SoupSession rather than per-backend Use a global SoupSession rather than a per-backend so that idle connections are reused appropriately. This prevents a process running out of file descriptors due to connections stuck in a CLOSE_WAIT state. It also has the side-effect of limiting the maximum number of concurrent connections because libsoup's max-conns property is respected, fixing bug 576460.
The first patch is an update of one from bug 708306 to use a single SoupSession. At this stage, I set ssl-strict to FALSE to prevent any behavior changes. Ideally, we'd have some way of configuring this, but that is for bug 708306. These patches don't fix the problem of leaking backends, but we have bug 509609 open for that.
Review of attachment 279718 [details] [review]: Looks good to me, thanks!
Review of attachment 279719 [details] [review]: Soup reference manual says: the primary reason you might need multiple sessions is if you need to have multiple independent authentication contexts. Are you sure we don't cause any authentication problems with this change? ::: daemon/gvfsbackendhttp.c @@ +686,3 @@ + the_session = soup_session_new_with_options ("user-agent", + "gvfs/" VERSION, + NULL); Wrong indentation...
I've seen during testing following warning: (process:16527): libsoup-WARNING **: (soup-uri.c:604):soup_uri_copy: runtime check failed: (SOUP_URI_IS_VALID (uri)) not sure it isn't connected with those changes, try to find reproducer.
Created attachment 279828 [details] [review] dav: don't set NULL path to avoid warnings From the soup reference manual: path is always non-NULL. For http/https URIs, path will never be an empty string either; if the input URI has no path, the parsed SoupURI will have a path of "/".
(In reply to comment #10) > Review of attachment 279719 [details] [review]: > > Soup reference manual says: the primary reason you might need multiple sessions > is if you need to have multiple independent authentication contexts. > > Are you sure we don't cause any authentication problems with this change? It should be fine because the http backend doesn't support authentication. The dav backend does but it uses one backend per process so each SoupSession is only used for one authentication context. I have tested authenticating to two separate shares concurrently and it worked OK. > > ::: daemon/gvfsbackendhttp.c > @@ +686,3 @@ > + the_session = soup_session_new_with_options ("user-agent", > + "gvfs/" VERSION, > + NULL); > > Wrong indentation... Will fix and commit.
Pushed to master as 0d32f60776b98ea360f0c0a452feaea4ae172340. Thanks for the reviews!
Review of attachment 279828 [details] [review]: Looks good. Please commit and close the bug. (I'll note here that reproducing was as simple as running gvfs-mount dav://localhost:8080/dav and watching the output of the dav backend)
Comment on attachment 279828 [details] [review] dav: don't set NULL path to avoid warnings commit 1e460863f255ad48895424d87adc5381098661f7
(In reply to comment #16) > (From update of attachment 279828 [details] [review]) > commit 1e460863f255ad48895424d87adc5381098661f7 I found that this commit actually fixes a problem as well rather than just silencing a warning. Previously it was not possible to mount two webdav shares on the same host with different paths, e.g. dav://localhost/ross and dav://localhost/common because the path was not being saved properly. Awesome!