search

google / sagetv

SageTV is a cross-platform networked DVR and media management system
http://forums.sagetv.com/
Apache License 2.0
267 stars 173 forks source link

Locking issues (lock() not called but unlock() will eventually be called) #333

Closed stuckless closed 7 years ago

stuckless commented 7 years ago

There are some issues with subtitles being reported on the forums. It may be related to this. I found 2 places where .lock() is not called, but .unlock() is called.

https://forums.sagetv.com/forums/showthread.php?p=607593&postcount=36

Also for the ReentrantReadWriteLock let's use NonFair locking instead of Fair locking as explained in this article on using the Locking mechanisms.

https://www.ibm.com/developerworks/java/library/j-jtp10264/

enternoescape commented 7 years ago

That's an easy one to miss. I would just add .lock() to both locations and be done with it. The mistake is a fairly easy one to make. The reason Fair locking was being used was to be consistent with our choice for Read/Write locking used for the Wizard (https://github.com/google/sagetv/blob/master/java/sage/Table.java#L38). I have myself played around with using NonFair, but the performance difference wasn't even measurable. Also in this case, I chose read to reduce contention between getting new subtitles from SRT files and returning the one needed right now. Otherwise you could end up missing random subtitles just because they couldn't be returned fast enough.

enternoescape commented 7 years ago

For the record, I do not contest that there's isn't more involved in Fair locking or that it's less efficient, but in our case, it doesn't make a meaningful impact.

stuckless commented 7 years ago

Yeah, I agree it's an easy one make, and just as easy to miss in a review. I Looked at that file about 10 times and still didn't see the issue. It wasn't until I did a search for subtitleLock usages and looked a the list returned that it really stood out that .lock() was missing :)

I'm ok to leave the locking to Fair, especially given your research into it as well. And while I've never needed to use ReadWrite lock features in Java, this does appear to be a case that would likely benefit from it.

enternoescape commented 7 years ago

I mostly use these kinds of locks when I also want to be able to control under some circumstances what happens when a lock can't be acquired immediately or within a time frame. You generally have a lot more options.

enternoescape commented 7 years ago

I did a search similar to what you did and I only found the same two spots you did. This should clean things up. The funny thing is I'm fairly certain I tested VOB subtitles and didn't run into this issue. I don't understand why not.

stuckless commented 7 years ago

Thx for fixing.