ejeschke / ginga

The Ginga astronomical FITS file viewer
BSD 3-Clause "New" or "Revised" License
121 stars 77 forks source link

Contents recreate_toc() and switch_image3 can have race condition #80

Closed pllim closed 9 years ago

pllim commented 9 years ago

I noticed this when I had 20 multi-extension FITS loaded at the same time. If Contents attempts to perform image switching operation while table of contents is being populated, I get this:

  File ".../lib/python2.7/site-packages/ginga-2.2.20150212221817-py2.7.egg/ginga/qtw/plugins/Contents.py", line 86, in switch_image3
    self.switch_image2(items[0], 0)
IndexError: list index out of range

This is most likely the cause:

    def recreate_toc(self):
        self.logger.debug("Recreating table of contents...")
        toclist = list(self.nameDict.keys())
        toclist.sort()

        self.treeview.clear()

# Somewhere here, switch_image3() was invoked by a callback

        for key in toclist:
            ...

I am thinking to fix it like this:

    def switch_image3(self):
        items = list(self.treeview.selectedItems())
        if len(items) == 0:
            return
        self.switch_image2(items[0], 0)
ejeschke commented 9 years ago

@pllim, I think we'd better fix it with a threading reentrant lock. I'll take a look at it.

ejeschke commented 9 years ago

Could you try commit c4410d72c1244b4507374574ea78da8dc3671ceb ? This has a lock added to protect against this race condition.

pllim commented 9 years ago

Hmm. I am getting a Python traceback when I switch image via Thumbs

Traceback (most recent call last):
  File ".../lib/python2.7/site-packages/ginga-2.2.20150212221817-py2.7.egg/ginga/qtw/plugins/Thumbs.py", line 36, in mousePressEvent
    self.thumbs_cb()
  File ".../lib/python2.7/site-packages/ginga-2.2.20150212221817-py2.7.egg/ginga/qtw/plugins/Thumbs.py", line 101, in <lambda>
    image_future)
  File ".../lib/python2.7/site-packages/ginga-2.2.20150212221817-py2.7.egg/ginga/misc/plugins/ThumbsBase.py", line 170, in load_file
    image_future=bnch.image_future)
TypeError: add_preload() got an unexpected keyword argument 'image_future'
Traceback (most recent call last):
  File ".../lib/python2.7/site-packages/ginga-2.2.20150212221817-py2.7.egg/ginga/qtw/plugins/Thumbs.py", line 36, in mousePressEvent
    self.thumbs_cb()
  File ".../lib/python2.7/site-packages/ginga-2.2.20150212221817-py2.7.egg/ginga/qtw/plugins/Thumbs.py", line 101, in <lambda>
    image_future)
  File ".../lib/python2.7/site-packages/ginga-2.2.20150212221817-py2.7.egg/ginga/misc/plugins/ThumbsBase.py", line 170, in load_file
    image_future=bnch.image_future)
TypeError: add_preload() got an unexpected keyword argument 'image_future'
ejeschke commented 9 years ago

Oops, forgot to test with image preloading. Fixed in 066ebe3dd723a8a9c8b9ba3b78b029d375718555. Could you give it another go? Tnx

pllim commented 9 years ago

I am not sure if the lock is locking stuff down. I added some print statements, namely 1 and 2 marks where treeview.clear() is invoked, and 10 and 20 marks where switch_image2() happens. I get the following:

1 <_RLock owner='MainThread' count=1>
10 <_RLock owner='MainThread' count=2>
Traceback (most recent call last):
  File ".../ContentsManager.py", line 70, in switch_image3
    self.switch_image2(items[0], 0)
IndexError: list index out of range
2 <_RLock owner='MainThread' count=1>

So the lock is keeping track of counts but does not stop the race condition. Any idea?

ejeschke commented 9 years ago

Hmm. Re-reading your suggested fix, it certainly seems like that is a proper check to do anyway. Can you give it a try and if it seems to be working, submit a PR? Tnx.