Created attachment 264392 [details] [review]
trash: Don't use try_ for blocking methods

Don't use the try_ variants of the GVfsBackend methods for methods that
could block.

Not sure about exact division to the try_ versions. Why do you think there shouldn't be the try_ versions? Does it cause problems somewhere?

gvfsbackend.h:
These try_ calls should be fast and non-blocking, scheduling the i/o
operations async (or on a thread) or reading from cache.

The recent backend which is quite similar also using the try_ methods. The try_ methods are used also for cached results e.g. in the gphoto2 backend. 

I think the trash backend offering "cached" results from the disk, so is quite fast, or am I wrong?

(In reply to comment #2)
> Not sure about exact division to the try_ versions. Why do you think there
> shouldn't be the try_ versions? Does it cause problems somewhere?
> 
> gvfsbackend.h:
> These try_ calls should be fast and non-blocking, scheduling the i/o
> operations async (or on a thread) or reading from cache.
> 
> The recent backend which is quite similar also using the try_ methods. The try_
> methods are used also for cached results e.g. in the gphoto2 backend. 
> 
> I think the trash backend offering "cached" results from the disk, so is quite
> fast, or am I wrong?

Well, as per the comment, try_ calls should not do i/o since they are executed as part of the main loop. Even doing local i/o, such as reading a block from disk can take some time, especially if the trash is on an nfs home directory.

The recent backend is indeed similar and I am working on a patch to fix it too.

While it probably doesn't cause any major problems in ordinary situations when using a local disk, the trash backend may hang if, for example, an nfs mount were to hang since the i/o operation is done as part of the main loop.

Ok, you are right, so please do the patch also for the recent backend...

Review of attachment 264392 [details] [review]:

It looks good.

Thanks for the review. Pushed to master as 052682c8a22bf8d7c86fb0f086a119dd7fec4c6b.

I have opened a bug 721908 for the recent backend.

note sure if that's due to this change but Ubuntu started receiving sigabrt in gvfsd-trash since 1.19.4
https://bugs.launchpad.net/ubuntu/+source/gvfs/+bug/1272679

" g_assertion_message (domain=domain@entry=0x0, file=file@entry=0x804f806 "trashdir.c", line=line@entry=218, func=func@entry=0x804f9e8 <__FUNCTION__.22163> "trash_dir_watch", message=message@entry=0xb4905968 "assertion failed: (dir->monitor == NULL)") at /build/buildd/glib2.0-2.39.3/./glib/gtestutils.c:2352
 g_assertion_message_expr (domain=domain@entry=0x0, file=file@entry=0x804f806 "trashdir.c", line=line@entry=218, func=func@entry=0x804f9e8 <__FUNCTION__.22163> "trash_dir_watch", expr=expr@entry=0x804f811 "dir->monitor == NULL") at /build/buildd/glib2.0-2.39.3/./glib/gtestutils.c:2367
 trash_dir_watch (dir=0x9375920) at trashdir.c:218
 trash_watcher_watch (watcher=0x936dda8) at trashwatcher.c:297
 trash_backend_get_dir_monitor (backend=0x9370858, create=create@entry=1) at gvfsbackendtrash.c:80
..."

I've opened https://bugzilla.gnome.org/show_bug.cgi?id=723305 about the issue from the previous comment

Yeah, it could well be... it's probably a race issue since the try_ version only allows one thread.  I'll revert the change for now since it wasn't critical.

Reopening since the patch was reverted so the original problem remains.

-- GitLab Migration Automatic Message --

This bug has been migrated to GNOME's GitLab instance and has been closed from further activity.

You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.gnome.org/GNOME/gvfs/issues/218.