Opened 9 years ago

Closed 9 years ago

#5168 closed Enhancement (fixed)

make libtransmission's public funcs nonblocking when possible

Reported by: jordan Owned by: jordan
Priority: Normal Milestone: 2.80
Component: libtransmission Version: 2.73
Severity: Normal Keywords:
Cc: mannionpatrick@…, thomas_reardon@…

Description (last modified by jordan)

Related to #1753 because it addresses some of the same symptoms.

The most common user-visible freezing in Transmission comes when the libtransmission thread is doing disk IO on piece data and the GUI thread tries to refresh its information on a torrent, such as with tr_torrentStat().

tr_torrentStat() currently requires a thread lock to ensure that it's not called at the same time that another thread is freeing the torrent. If we change tr_torrent to use a refcount-based life cycle, then we can modify tr_torrent's public accessor functions to keep a temporary refcount instead of requiring a thread lock.

So the advantage is that the Mac and GTK+ GUIs should freeze less often, even without #1753.

A possible disadvantage is that one of the nonblocking calls might return an old value if another thread's modifying the value at the same time. IMO this is acceptable because the torrent accessor functions are called periodically to refresh the client's GUI, so incorrect data will be fixed on refresh.

New public functions:

  • tr_torrentRef()
  • tr_torrentUnref()

Removed public functions:

  • tr_torrentFree()

Functions made nonblocking:

  • tr_torrentStat()
  • tr_torrentGetSpeedLimit_KBps()
  • tr_torrentUsesSpeedLimit()
  • tr_torrentUsesSessionLimts()
  • tr_torrentGetRatioLimit()
  • tr_torrentGetSeedRatio()
  • tr_torrentGetFilePriorities()
  • tr_torrentPeers()
  • tr_torrentTrackers()
  • tr_torrentWebSpeeds_KBps()
  • tr_torrentAvailability()
  • tr_torrentAmountFinished()

Change History (10)

comment:1 Changed 9 years ago by jordan

  • Component changed from Transmission to libtransmission
  • Owner set to jordan
  • Status changed from new to assigned

comment:2 Changed 9 years ago by jordan

  • Description modified (diff)

comment:3 Changed 9 years ago by jordan

I've landed a first cut of this in r13651. This might or might not break the Mac build depending on how heavily it relies on tr_torrent pointers to be const...

comment:4 Changed 9 years ago by mannionp

  • Cc mannionpatrick@… added

comment:5 Changed 9 years ago by reardon

  • Cc thomas_reardon@… added

comment:6 Changed 9 years ago by jordan

There are a couple of fixes for this in r13652 and r13653 in trunk... if anyone using this in the nightlies (it's not in 2.75) has comments / bug reports on this diff, I'd love to read them.

comment:7 Changed 9 years ago by reardon

Perhaps I misunderstand the contexts in which tr_torrentRef() and tr_torrentUnref() can be called, but how do you avoid a race condition without a spinlock or similar mutex on refCount? I'm imagining a call to tr_torrentRef() when tr_torrentUnref() has just decremented refCount to zero.

On the same point, in torrent.c it seems you check tr_torrentLock() at some points but not others, why? And does tr_torrentLock() have a similar racing problem (torrent is freed/nulled after tr_isTorrent() but just before sessionLock is obtained) to ref&unref, or is it protected because the caller always ensures that ref() is called before lock()?

Last edited 9 years ago by reardon (previous) (diff)

comment:8 Changed 9 years ago by jordan

These are good questions and I wonder if r13651, r13652, and r13653 are the wrong approach.

The two possible race conditions are:

  1. GUI thread calls tr_torrentRemove() while libtransmission thread calls tr_torrentStat() et al.
  1. libtransmission thread calls tr_torrentRemove() while GUI thread calls tr_torrentStat() et al.

I believe 1. is already addressed because tr_torrentRemove() simply sets an 'isDeleting' flag and defers the actual teardown back to the libtransmission thread.

I believe 2. is also already addressed. The only place that libtransmission calls tr_torrentRemove() is in rpcimpl.c's torrentRemove() function, as the result of an RPC request. But before removing the torrent, the libtransmission first asks the GUI for permission via rpcimpl.c's notify mechanism. Both the GTK+ and Mac clients refuse permission and defer the work to their own GUI thread, which takes us back to the previous race condition.

Unless I'm missing something, we can remove the locks from tr_torrentStat() et al. without needing these new ref/unref functions and without needing any other safety mechanisms.

comment:9 Changed 9 years ago by jordan

r13670, remove the tr_torrentRef(), tr_torrentUnref() functions as discussed in comment:8

comment:10 Changed 9 years ago by jordan

  • Resolution set to fixed
  • Status changed from assigned to closed
Note: See TracTickets for help on using tickets.