BF: allow enough time for all ioHub device._close() to finish#6540
Merged
peircej merged 2 commits intopsychopy:releasefrom Jun 10, 2024
Merged
BF: allow enough time for all ioHub device._close() to finish#6540peircej merged 2 commits intopsychopy:releasefrom
peircej merged 2 commits intopsychopy:releasefrom
Conversation
Member
|
You make a convincing argument and I'm pulling this in. We have a few weeks of testing ahead before a target release date for 2024.2.0 of 15 July, so we should have a little time to work out if this new approach is causing trouble (e.g. taking a long time to close etc). |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
TLDR:
This BF fixes the problem of
gevent.sleep()during greenlets foreign togevent.joinall()returning the server process during shutdown. --- this is achieved by adding a genericgevent.wait()at the end of the server process script.This BF gives all ioHub devices infinite amount of time to finish their processing --- this is achieved by removing the
timeout=5when waiting forComputer.iohub_processto finish duringioHubConnection._shutDownServer().I believe this change is in line with how PsychoPy waits for threads during
core.quit(). When things hang, people can always force shut the python session to exit anyways.Detailed explanation for reference:
There is currently a horse race problem at the end of an experiment when closing down
ioHubConnection. This is becauseioHubServeris started on a subprocess and uses greenlets viageventto allow different C stacks to get cooperatively scheduled. The problem is thatgevent.joinall(glets)here only explicitly joins the spawned greenlets. When thepsychopy-eyetracker-sr-researchcallsgevent.sleep(0.01)duringsetRecordingState(), it is in a strange position. It is not on the MAIN greenlet, but it still immediately yields back to thegeventhub the greenlet executing_close()and continues the ioHub server process script. This is likely becausegevent.joinall(glets)doesn't know about the greenlet. Since no other joined greenlet runs slower than the eyetracker_close()processing, thegevent.sleep()call completes thestart_iohub_process.pyexecution right away and returnsComputer.iohub_process.wait(). Thencore.quit()immediately kills the poor greenlets without mercy before they could call for help, so to speak.Since we now rely on ioHub server
_shutDownServer()to close all the ioHub devices properly and finish remaining processing (such as setting eyetracker recording and connection states toFalse, downloading the eyetracking data files, etc., etc.), the use oftimeout=5inComputer.iohub_process.wait()is no longer appropriate.This problem has likely existed in
ioHubfrom the beginning. In the demo coder scripts (here and here), we doat the end of an experiment. I don't think the
iohub_processsubprocess would wait fortracker.setRecordingState(False)to finish ifgevent.sleep()is called as a part of it. NOTE: this doesn't happen in thesetRecordingState()formouseGaze.Presumably
would take a bit longer than 500ms. There is no
gevent.sleep()called duringtracker.setConnectionState(False), so the subprocess will wait for it to return. However, if.setConnectionState(False)takes more than 5 seconds (such as due to downloading a large eye tracking data file), thecore.quit()will still kill it.Currently in Builder we don't make explicit call to close eye trackers like in the example coder scripts. There was an attempt to do so, which I removed in #6526 since
deviceManager.removeDevice('eyetracker')does not actually close the eye tracker without.close()defined for ioHubEyeTrackerDevice. Of course, even if we were to expose a.close()function, this would become one greenlet, which thegevent.sleep(0.01)during.setRecordingState(False)would yield, i.e., the ioHub subprocess will not wait for.close()to return.The problem is therefore twofold:
The reason why
timeout=5is problematic is because ioHub devices indeviceManager.devicesare exposed asioHubDeviceViewinstances and communicated with requests via UDP sockets. Whenever we do a tracker dot function call, it is handled as sending a request to the ioHub subprocess and that line immediately returns. The main process of PsychoPy carries on. So even if the ioHub subprocess waits (by making additional UDP requests such as.setConnectionState(False), or even just addingtime.sleep()in_close()), when things take longer than 5s,ioHubConnection._shutdownSever()will still kill it because of theComputer.iohub_process.wait(timeout=5).In summary, I don't think it is feasible to close down eye trackers safely during
core.quit()with theComputer.iohub_process.wait(timeout=5)in there. All function calls to the eyetracker as anioHubDeviceViewinstance is made through UDP requests to the ioHub server running on theiohub_processsubprocess. So everything running longer than 5 seconds will get killed by thetimeout. The cleanest way is to remove thetimeoutand ask the main psychopy process to wait for theiohub_processto return, just like how it waits for other threads to close.