GNOME Bugzilla – Bug 511802
Backends cannot initiate clean shutdown
Last modified: 2013-07-08 15:16:21 UTC
There's currently no way for backends to "commit suicide". The use cases would be: - Network shares going away when the network dies - ObexFTP listings going away when the Bluetooth dongle disappears - CD is ejected
Retitling due to possibly offensive title ;)
What I'm doing in the gphoto2 and cdda backends right now (in patches to be committed to trunk) is to call exit() when the device / media goes away. Works fine for now... but we need a better way.
That's working pretty well for me as well. But I'm pretty sure that we should have a way to tell the daemon that we exited cleanly, as opposed to having crashed.
Created attachment 107454 [details] [review] Patch This patch adds a way for the backends to exit cleanly. With the current code both daemon and backend finalize methods are never called. I've only tested it with sftp and cdda backends, if the patch is ok I'll fix the other backends too.
I think this is mostly right. The only thing missing is that backends with an open file on them will not go away until the last file on the backend is closed, as the open files (GVfsChannels) are also sources, and ref the backend. We should probably introduce g_vfs_backend_shutdown() which force-closes all sources related to the backend and then emits source_closed.
Just fyi there is a $200 bounty being offered on this bug. See http://www.freedomsponsors.org/core/issue/75/nautilus-sftp-connection-breaks-spontaneously-after-a-while-needing-re-login-to-fix-it That is if I'm correct in assuming this is the fix for Nautilus thinking a connection is still active.
Created attachment 239948 [details] [review] Adds-a-way-for-the-backends-to-exit-cleanly Rebased the original patch and use g_vfs_daemon_close_active_channels() which was added in 2011 to force-close all sources related to the backend and then emit source_closed
Created attachment 240463 [details] [review] Adds-a-way-for-the-backends-to-exit-cleanly Updated patch to add functionality to g_vfs_channel_force_close() so that cdda backend deals with open files.
Review of attachment 240463 [details] [review]: Hmm, looking at this I don't quite see how this would unmount the mount on error. Like, take the sftp part, it will close all active channels, i.e. all open files, but the actual mount is still a JobSource attached to the daemon, so it will not go idle and exit. Doesn't it need to call g_vfs_job_source_closed() on the backend. Also i think g_vfs_daemon_close_active_channels() needs to be changed to take a GVfsBackend argument and only close channels with that backend. As a single daemon may handle multiple mounts/backends. The combination of source_closed + close_active channels calls should probably be put in a shared helper function like g_vfs_backend_unclean_shutdown() or something. ::: daemon/gvfsbackendsftp.c @@ +1284,3 @@ + { + /* EOF */ + return; Is this really right? Is it ever not a closed connection if write returns 0 (for a non-zero count)? ::: daemon/gvfschannel.c @@ +789,3 @@ + op_job, + op_job->handle); + } This seems wrong for many reasons: * It assumes the handle is for reading, but it may be a write handle * It calls directly into the backend outside the job, rather than letting the job do it * It does sync i/o on the main thread * It tries to do operations on an backend at a state where it is either forced unmounted or uncleanly shutdown. At this point we can't assume that the backend functions. What is the exact problem this is trying to handle?
First of all thanks for your time and feedback. Second please forgive any stupid assumptions I have made as my knowledge of gvfs is the result of two afternoons of hacking around in my spare time. (In reply to comment #9) > Review of attachment 240463 [details] [review]: > > Hmm, looking at this I don't quite see how this would unmount the mount on > error. > > Like, take the sftp part, it will close all active channels, i.e. all open > files, but the actual mount is still a JobSource attached to the daemon, so it > will not go idle and exit. Doesn't it need to call g_vfs_job_source_closed() > on the backend. g_vfs_daemon_close_active_channels() has a loop that for each channel calls g_vfs_channel_force_close() which then calls g_vfs_job_source_closed() > Also i think g_vfs_daemon_close_active_channels() needs to be > changed to take a GVfsBackend argument and only close channels with that > backend. As a single daemon may handle multiple mounts/backends. Ok, I didn't know that it worked like this. I assumed from the calls to exit() in cdda that there were multiple instances of the daemons. > The combination of source_closed + close_active channels calls should probably > be put in a shared helper function like g_vfs_backend_unclean_shutdown() or > something. If I was to make a version of g_vfs_daemon_close_active_channels() that accepted GVfsBackend as an argument as you have suggested I think this would be enough wouldn't it? As stated above g_vfs_channel_force_close() calls g_vfs_job_source_closed() > > ::: daemon/gvfsbackendsftp.c > @@ +1284,3 @@ > + { > + /* EOF */ > + return; > > Is this really right? Is it ever not a closed connection if write returns 0 > (for a non-zero count)? I'm really are not sure about this I simply took this from Carlos patch I'm open to any suggestions here. > > ::: daemon/gvfschannel.c > @@ +789,3 @@ > + op_job, > + op_job->handle); > + } > > This seems wrong for many reasons: Again forgive me for making stupid assumptions here. I will try to explain why I did things this way. > * It calls directly into the backend outside the job, rather than letting the > job do it I assumed that if I used a job to do this that it might never be called as the next lines in g_vfs_channel_force_close() cancel the current job and from what I can tell clear the que of further jobs (maybe I'm misunderstanding whats going on here?) > * It assumes the handle is for reading, but it may be a write handle Yes this came across my mind after I had submitted this patch this is probably something I do need to consider. > * It tries to do operations on an backend at a state where it is either forced > unmounted or uncleanly shutdown. At this point we can't assume that the backend > functions.' As far as I understand from the code in gvfsdeamon.c job_source_closed_callback() the exit/finalize calls are not made until after *all* the sources are closed. So the backend would only be shutdown after these operations are preformed. Again maybe I'm not understanding this correctly? if (daemon->job_sources == NULL) daemon_schedule_exit (daemon); > * It does sync i/o on the main thread > > What is the exact problem this is trying to handle? From what I could tell the code in g_vfs_channel_force_close() was more for closing read/write streams from a network socket and wasn't enough to deal with closing open files on the cdda backend. gvfsbackendcdda.c do_close_read() seems to be needed to be called to deal with having such files open e.g. when the cd tray it manually ejected. Maybe there is a better place to call it? Or a better way to deal with this situation? I will repeat again I'm not expert on any of this so I welcome any suggestions/corrections.
(In reply to comment #10) > > Hmm, looking at this I don't quite see how this would unmount the mount on > > error. > > > > Like, take the sftp part, it will close all active channels, i.e. all open > > files, but the actual mount is still a JobSource attached to the daemon, so it > > will not go idle and exit. Doesn't it need to call g_vfs_job_source_closed() > > on the backend. > > g_vfs_daemon_close_active_channels() has a loop that for each channel calls > g_vfs_channel_force_close() which then calls g_vfs_job_source_closed() Yeah, but it only closes channels, i.e. open files. Other sources such as GVfsBackends are not closed. > > Also i think g_vfs_daemon_close_active_channels() needs to be > > changed to take a GVfsBackend argument and only close channels with that > > backend. As a single daemon may handle multiple mounts/backends. > > Ok, I didn't know that it worked like this. I assumed from the calls to exit() > in cdda that there were multiple instances of the daemons. It depends on the backend. Some backends run one process per mount, some handle multiple mounts in a single daemon (http does this for example). > > The combination of source_closed + close_active channels calls should probably > > be put in a shared helper function like g_vfs_backend_unclean_shutdown() or > > something. > > If I was to make a version of g_vfs_daemon_close_active_channels() that > accepted GVfsBackend as an argument as you have suggested I think this would be > enough wouldn't it? As stated above g_vfs_channel_force_close() calls > g_vfs_job_source_closed() There is no reason for g_vfs_daemon_close_active_channels() to *not* take a backend, even for the current callers of it. We should just fix it. But, that doesn't actually close the backend object itself, as per the above. > > > > ::: daemon/gvfsbackendsftp.c > > @@ +1284,3 @@ > > + { > > + /* EOF */ > > + return; > > > > Is this really right? Is it ever not a closed connection if write returns 0 > > (for a non-zero count)? > > I'm really are not sure about this I simply took this from Carlos patch I'm > open to any suggestions here. We should test it i guess. See if it happens at all. > > > > ::: daemon/gvfschannel.c > > @@ +789,3 @@ > > + op_job, > > + op_job->handle); > > + } > > > > This seems wrong for many reasons: > > Again forgive me for making stupid assumptions here. I will try to explain why > I did things this way. > > > * It calls directly into the backend outside the job, rather than letting the > > job do it > > I assumed that if I used a job to do this that it might never be called as the > next lines in g_vfs_channel_force_close() cancel the current job and from what > I can tell clear the que of further jobs (maybe I'm misunderstanding whats > going on here?) Yes, it is true that no further jobs will be handled. But the *reason* for this is generally that the backend is in a broken state so we need to shut it down. For instance, the sftp backend does this if the connection to the underlying ssh process we're talking to went away (reported an error writing commands). At this point its unlikely that any calls into the backend, including "close an open file" will do anything useful, so we're forcefully tearing down any open files on the mount. > > * It tries to do operations on an backend at a state where it is either forced > > unmounted or uncleanly shutdown. At this point we can't assume that the backend > > functions.' > > As far as I understand from the code in gvfsdeamon.c > job_source_closed_callback() the exit/finalize calls are not made until after > *all* the sources are closed. So the backend would only be shutdown after these > operations are preformed. Again maybe I'm not understanding this correctly? Sure, the *object* lives, but it won't work (i.e. the ssh process has died). > > What is the exact problem this is trying to handle? > > From what I could tell the code in g_vfs_channel_force_close() was more for > closing read/write streams from a network socket and wasn't enough to deal with > closing open files on the cdda backend. gvfsbackendcdda.c do_close_read() seems > to be needed to be called to deal with having such files open e.g. when the cd > tray it manually ejected. Maybe there is a better place to call it? Or a better > way to deal with this situation? I will repeat again I'm not expert on any of > this so I welcome any suggestions/corrections. g_vfs_channel_force_close() closes the socket that apps use to read/write data to that particular file. Since we shut them down all apps will get proper error reported if they continue reading from that file, and we will get no more operations from them. It does nothing for the backend file object itself, true. However, a "normal" close backend operation is not the right thing to do here anyway (as per the sftp example above). Its possible we could add some kind of force_file_close operation on the backend, with a proper job for it. However, its not really that critical for the current usecases, as the mount daemon exiting should be enought to clean things up.
> Yeah, but it only closes channels, i.e. open files. Other sources such as > GVfsBackends are not closed. Thanks this is what I was missing, everything makes a lot more sense now. I couldn't figure out how the Backends were meant to be closed after you said there could be more than one. In the end it was there in the original patch all along. I'm pretty sure this will also fix the problem I was trying to solve by calling close_read. I will create a couple of patches over the next couple of days. One to fix up g_vfs_daemon_close_active_channels() and one for this bug. Again thanks for your patience.
Created attachment 241010 [details] [review] Fix g_vfs_daemon_close_active_channels() to take a GVfsBackend argument Fix g_vfs_daemon_close_active_channels() to take a GVfsBackend argument and only close channels with that backend as a single daemon may handle multiple mounts/backends. I'm just a little worried about this line: g_vfs_channel_get_backend (G_VFS_CHANNEL (l->data)) == backend) Is it valid to compare GObjects like this? I have seen it done elsewhere in the code e.g GDBusConnection comparison. I've spent quite a bit of time looking for documentation on this but in the end I found nothing so I thought I would just post the patch and get some feedback. Tim
Review of attachment 241010 [details] [review]: looks good. pushed this.
And yes, GObjects instance equality can be done with "==".
(In reply to comment #14) > Review of attachment 241010 [details] [review]: > > looks good. pushed this. Doesn't the g_vfs_daemon_close_active_channels() declaration need to be changed in the header too?
(In reply to comment #16) > (In reply to comment #14) > > Review of attachment 241010 [details] [review] [details]: > > > > looks good. pushed this. > > Doesn't the g_vfs_daemon_close_active_channels() declaration need to be changed > in the header too? Yes it does this is what happens when you try to create a patch while on public transport. Sorry about that.
(In reply to comment #17) > > Yes it does this is what happens when you try to create a patch while on > public transport. Sorry about that. Are you taking care of fixing this then? If you're travelling I can do it.
I would be greatful if you could do it as I dont have access to a computer for the next hour and I don't have git access anyway thanks Colin.
https://git.gnome.org/browse/gvfs/commit/?id=05ebb30a6a4e3aa5a30355a594fb53ece3059f09
Force-closing active channels is one thing and is fine in any case. Emitting and processing the shutdown signal is probably fine for the case of one mount per process but not for a shared process. What I would like to see is telling the mount tracker the mount is going away. For the moment this is handled either by going through GVfsJobUnmount (calling org.gtk.vfs.MountTracker.UnregisterMount()) or by detecting lost name owner on the tracker side, typically when backend exits. If neither is done, the mount is treated as active. Btw. historically there was no UnregisterMount() method available and backend was treated as unmounted (from UI point of view) once the process exited. Calling g_vfs_backend_unregister_mount() should do the trick from backend side (see gvfsjobunmount.c for example how to use it, we should probably wrap the dbus method callback and only return result + GError, I was too lazy to do that during porting to GDBus...). After that it's safe to call exit() for non-shared backends.
(In reply to comment #21) > Force-closing active channels is one thing and is fine in any case. Emitting > and processing the shutdown signal is probably fine for the case of one mount > per process but not for a shared process. When you say shared process are you talking about, multiple backends for a single daemon? If so the shutdown signal will only be given when all backends have been closed (the daemon checks if there are still sources before issuing it). Or are you taking about something else? If so are there any architectural/design documents somewhere so I can get my head around how GVFS actually work? > What I would like to see is telling the mount tracker the mount is going away. > For the moment this is handled either by going through GVfsJobUnmount (calling > org.gtk.vfs.MountTracker.UnregisterMount()) or by detecting lost name owner on > the tracker side, typically when backend exits. If neither is done, the mount > is treated as active. Btw. historically there was no UnregisterMount() method > available and backend was treated as unmounted (from UI point of view) once the > process exited. > > Calling g_vfs_backend_unregister_mount() should do the trick from backend side > (see gvfsjobunmount.c for example how to use it, we should probably wrap the > dbus method callback and only return result + GError, I was too lazy to do that > during porting to GDBus...). After that it's safe to call exit() for non-shared > backends. Ok this sounds fair enough. Does this mean I would need to change unregister_mount_callback() to accept a backend as valid user_data also and ignore the send_reply calls in this instance? As we would be calling it directly from the backend.
(In reply to comment #21) > Force-closing active channels is one thing and is fine in any case. Emitting > and processing the shutdown signal is probably fine for the case of one mount > per process but not for a shared process. The shutdown signal is not emitted by the backends. Its only sent by the daemon itself when its idle (no sources for a while), so it seems safe to me even for the shared process case. > What I would like to see is telling the mount tracker the mount is going away. > For the moment this is handled either by going through GVfsJobUnmount (calling > org.gtk.vfs.MountTracker.UnregisterMount()) or by detecting lost name owner on > the tracker side, typically when backend exits. If neither is done, the mount > is treated as active. Btw. historically there was no UnregisterMount() method > available and backend was treated as unmounted (from UI point of view) once the > process exited. True, this is kind of important for the shared process case.
(In reply to comment #22) > When you say shared process are you talking about, multiple backends for a > single daemon? If so the shutdown signal will only be given when all backends > have been closed (the daemon checks if there are still sources before issuing > it). Ah right, I see it now, alright then. > If so are there any architectural/design documents somewhere so I can get my head around how GVFS actually work? Not yet but I'm about to write documentation in coming weeks. I'm constantly learning something new such as this case. > Ok this sounds fair enough. Does this mean I would need to change > unregister_mount_callback() to accept a backend as valid user_data also and > ignore the send_reply calls in this instance? As we would be calling it > directly from the backend. I meant it by using g_vfs_backend_unregister_mount() somewhere else without any changes to gvfsjobunmount.c. You can actually pass NULL for the callback if you don't need to know the method call result but since it's async, ensuring that the d-bus message has been sent out is a good idea before continuing in shutting down (one may also want to call g_dbus_connection_flush_sync() at a proper place).
Created attachment 241333 [details] [review] Adds infrastructure for the backends to exit cleanly I decided to split out the shut-down infrastructure into its own patch for a couple of reasons. 1. There is no reason to continually review this code with each new patch. 2. Its possible that this patch might be useful to commit without the backend updates as the existing unregister_mount calls might already exercise these code paths.
Comment on attachment 241333 [details] [review] Adds infrastructure for the backends to exit cleanly Attachment 241333 [details] pushed as 517ae68 - Adds infrastructure for the backends to exit cleanly
Created attachment 241981 [details] [review] Final puzzle pieces Adds remaining shutdown infrastructure and call from cdda/sftp backends. I have successfully tested the cdda backend. I have not tested the sftp backend but I'm waiting on feedback from an upstream patched build: https://bugs.launchpad.net/ubuntu/+source/gvfs/+bug/377322
Created attachment 242387 [details] [review] Adds remaining infrastructure and call from cdda backend I have attached a patch that adds the remaining shutdown infrastructure and only calls it from the cdda backend. I will submit the patch for the sftp backend once it has been confirmed as working correctly.
Any chance of getting this latest patch reviewed? It would be good to get it in so other people could start working on patches for other backends. I wasnt sure what Tomas meant by "we should probably wrap the dbus method callback and only return result + GError" or where/if I should be calling g_dbus_connection_flush_sync()
Sorry for the long delay, but the patch looks good to me, so i pushed it to master. Got any feedback on the sftp side?
Created attachment 243475 [details] [review] Implement-clean-shutdown-in-sftp-backend I was hoping the sftp patch might magically help fix bug 515217 but it didn't. However no regressions were reported, I also managed to test it out a bit myself once I played with a few on my router settings. Anyway here it is.
Looks good to me, i pushed it. I guess afc, obexftp and mtp are left that can use the same fix. Thanks a lot for doing this work.
Created attachment 243579 [details] [review] Implement-clean-shutdown-in-afc-backend (In reply to comment #32) > I guess afc, obexftp and mtp are left that can use the same fix. Ok, you talked me into it. Note these patches are untested so I've split them into individual patches. > Thanks a lot for doing this work. No worries.
Created attachment 243580 [details] [review] Implement-clean-shutdown-in-obexftp-backend
Created attachment 243582 [details] [review] Implement-clean-shutdown-in-mtp-backend
Bump. Any chance for a review of these patches so this bug can finally be closed?
Created attachment 247079 [details] [review] Implement-clean-shutdown-in-afc-backend v2 Fix compilation warnings
Created attachment 247080 [details] [review] Implement-clean-shutdown-in-mtp-backend v2 fix compilation warnings
Created attachment 247081 [details] [review] Implement-clean-shutdown-in-obexftp-backend v2 fix compilation warnings
Patches looks good to me, so I've commited it to master... commit 53909ed97cdf84059bf16d8dde18f8183529d788 commit 91bd02a0e3216d7b901405b7636055881867bf92 commit 5354d7ff2b3ade6f2d78769cbb831ee861572e13 Thank you for your patches, so we can now finally close this bug.