[PATCH] Allow moves to be done asynchronously

[tiennou]

This patch adds the API needed in libT to handle async moves.

Right now the OS X GUI builds, and works. Others should not be affected (YMMV). The only issue I have is (as the comment in torrent.c:2627 points out) that single-torrent files won't update their progress until they are done, which is especially visible for large torrents.

​http://github.com/tiennou/transmission/tree/async-move

Feel free to review the code either there or directly on GitHub?.

[charles]

I don't understand where the asynchronous part of this patch comes in. It seems to still move the files in exactly the same way it used to.

[tiennou]

I've rebased the branch against latest master (​cool diff view).

I've indeed changed nothing to the code that move files, but moves progression are reported through tr_stat instead of setme_ ptrs, (heavily inspired from the verification code), which removes the need from locking the UI thread (​comment) while a move is in progress (that's obviously OSX specific, but I guess the GTK GUI does the same thing).

So see this as just an API cleanup of tr_torrentSetLocation ;-). Only issue (as pointed in the ticket description) is that single-file torrent won't benefit from the reporting (see ​comment), since the tr_stat struct is updated after each file is successfully moved.

[charles]

I'm sorry, but I'm still confused. It sounds like this is worse than the current scheme, because now there's a thread contention issue between tr_torrentSetLocation() and tr_torrentStat() trying to get a lock on the libtransmission thread. Am I misunderstanding this?

[tiennou]

I'm not sure there's any thread contention issue, but I can't say for sure… I was under the impression that the event thread wasn't forced to lock anything, but it looks like the verification code does lock in verifyTorrent so I'm not sure anymore :-S.

I'll try to walk you through the new code path :

• The client asks libT to move a torrent via tr_torrentSetLocation(). This sets up a temp struct with a copy of the torrent id, the new path, and whether it should make a move or a copy (a.k.a delete the old files vs keep them). It then schedule setLocation() in the event thread and returns. This frees the GUI from having to lock the main thread while the copy happen (see #2117, #2012, #651, all duplicates of the issue this patch fixes).
• The event thread executes setLocation() : it does its thing in exactly the same way as before (calls tr_moveFile), but it updates the new tr_torrent fields via tr_torrentSetMoveState (new API). Each time tr_moveFile returns (see note below), setLocation updates the move progress. If an error happen while moving, it sets the local error to the strerror, and set the state to TR_MOVE_ERROR (which allow the client to display it). If there were no error, it sets the state to TR_MOVE_NONE, cleanup, and returns.
• The client calls tr_torrentStat() regularly. It locks the torrent, copies various fields (including the new move-related ones) from tr_torrent to tr_stat, and unlocks.

Note : this poses an issue with torrents with big files, pretty visible with single-file torrent, but it can also be seen in other cases. The move will start at TR_MOVE_MOVING/0.0, jump to TR_MOVE_MOVING/1.0 after tr_moveFile returns and immediately jump to TR_MOVE_NONE/0.0 since there's nothing left to move, which looks weird in the GUI (display "Moving… 0% and then jumps to "Waiting" or whatever state it was before the move without the progress bar ever showing progress. Fixing this requires tweaking tr_moveFile since it needs to report the current progress dynamically.

I hope this is a clearer explanation ;-). I'll be lurking in the IRC channel today if you want to discuss this.

[jordan]

I've tried to understand this patch but I still don't see how this makes things any better... it seems to just make them different.