[hotfix] Check element exists before removing for Python thread-safe queue

[20wildmanj]

Description

Fixes issue found in Tango deployments that use the default Python thread-safe queue instead of TangoRemoteQueue, where it was possible for remove to be called to remove an element already removed from a queue, causing an unhandled exception.

This hotfix merely will check to see if the element exists in the queue before calling the remove method, which is consistent with TangoRemoteQueue, which does not raise an exception in the same scenario as well.

Testing

• set USE_REDIS=False in config.py
• Run python -m unittest tests/testJobQueue.py, see all tests run successfully
• run only restful tango
• try uploading files to autograded assignment, see that they get graded properly

[cg2v]

While this will certainly prevent exceptions, is it actually the right fix? This looks like another variant of #207

[20wildmanj]

Yes we have looked at #207, though because my exposure to Tango has only been through hotfixes the past week I'm not 100% in terms of understanding the states of Tango. There's definitely a better solution, but making it happen right now may be difficult in terms of time.

[cg2v]

I'm also concerned because the exception I encountered was in assignJob, which only has one callsite. How was jobQueue.assignJob called on a job that was not in the unassigned queue?

[michellexliu]

The jobQueue.assignJob replaced job.makeAssigned() in __manage in #227 to fix a bug where the vm was never getting set, leading to issues in our autolab-docker installation.

[cg2v]

Upon further review of the code

• _manage is the only non-test caller of makeAssigned
• At that point in _manage, the job is never in the unassigned queue (redis or no redis), since getNextPendingJob removed it
  • If there will never be other callers of makeAssigned, then makeAssigned does not need to remove the job from the unassigned queue.
• despite their names, makeAssigned and makeUnassigned are not really complimentary operations. One is part of the normal workflow, the other is part of error recovery. most jobs will never have makeUnassigned called on them
• nontheless, this patch makes the two implimentations of TangoQueue work similarly

[20wildmanj]

Thanks for looking into this, I've removed the removal from the unassigned queue in assignJob.

[20wildmanj]

Closing this PR for now, will return to this issue with a better fix later.