Comment 3 for bug 133139

I've been looking at this bug a little bit, and need a bit of feedback... if any mentors have time :)

The problem is pretty straight forward, but the solution isn't (to me anyway!). As noted above, the problem is that the two calls to on_treeview_update_cursor_changed get stacked (rather than using callbacks), the second (more recent) call has to finish before the first call gets to check whether it's own lock has been released (a separate lock is created each time the function is called). Once the second call completes, the first call then gets a chance to check it's lock, finds it released and updates the UI, leaving the UI in an inconsistent state (change log displayed for previously selected item rather than current.)

Now as far as I can tell, the locks aren't being used for any thread-related concurrency issues (as I said, a separate lock is created for each new thread), but rather to wait until the thread finishes so that the calling process can update the UI.

So, the two solutions that I can see are:

1) Pass a callback function to the newly created thread and allow the creating process to finish (rather than waiting for the lock to be released). When the callback is called (with the package name as parameter), it checks to ensure that the package name being updated is actually the most recently requested package before it updates the UI. Eg:

def cache_updated_callback(package_name):
    if package_name == self.last_requested_package_name:
        # Update UI

This no longer requires the locks (unless I missed something!) but does require a new attribute (last_requested_package_name - could be wrapped up in a new changes_log object that manages its own states?) to ensure that the UI is only ever updated with the most recently requested package change log. Also means that cancelled requests can still update the cache with their data, even though they're not yet used in the UI (handy if user then clicks back on previously selected package).

On the downside, I'm guessing (from some of the comments in the code) the reason a callback wasn't used in the first place is because it leaves the newly created thread updating the UI, rather than the main process. But i don't think it would be difficult to do this in a thread-safe way as outlined above?

2) A second solution could be to continue using the locks/wait, but assign the lock to an attribute so that it can be used to cancel any previous requests. That is, whenever the function wants to create a new thread, it checks to see if there has been any previous requests (ie. there is a lock), and if so, releases the previous lock so the previous request-thread will not update the cache (seems a bit inefficient, as the cache may as well be updated, even if the data isn't used *yet*, but that's the way the code is atm), then creates and assigns a new lock to the class attribute, continuing on.

That way, when the second request completes, the UI is updated (and the call is popped off the stack). The first request will then check it's lock, find it released, query the cache but find the cache doesn't contain it's required value and so skip updating the UI (as in the current implementation).

Let me know which solution is preferable (I'm for (1) - the only reason I haven't just gone and coded it is a comment in get_changelog:
        # don't touch the gui in this function, it needs to be thread-safe
as calling the callback would mean that this function *is* updating the gui. I'm guessing the comment comes with more experience than I've got myself :-) )

Thanks for any help!
-Michael.