-
Notifications
You must be signed in to change notification settings - Fork 74
[#722] fix segfault and hung threads on KeyboardIinterrupt during parallel get #728
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
After a bit of manual testing, will attempt to make a proper test for SIGINT and SIGTERM to ensure things are left in an ok state. |
e9d96e2
to
7e9a09f
Compare
A GUI for example that maintains background asynch parallel transfers using PRC could trap and guard against Ctrl-C thusly:
|
abafff5
to
fb36836
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems reasonable. Just a couple of things in the test
import irods | ||
import irods.helpers |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It appears that we only actually use irods.helpers
. Could we remove the import irods
?
_clock_polling_interval = max(0.01, time.clock_getres(time.CLOCK_BOOTTIME)) | ||
|
||
|
||
def wait_till_true(function, timeout=None): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just in case... can we set the default timeout
to some high-ish value that is abundantly more than enough time to complete whatever transfer we are waiting for, but that will eventually fail if it is stuck? We can still support None
as "no timeout", but making it the default makes me squirm. The value I had in mind was like... 10 minutes?
test_case.assertEqual( | ||
process.wait(timeout=15), | ||
-sig, | ||
"Unexpected subprocess return code.", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please print the expected return code and the actual return code, if possible.
Looks like we have a conflict. Seems this PR is close to completion? |
Wherein we close down threads in an orderly way, so that things don't leave things to be disposed in the wrong order for the ever persnickety SSL shutdown logic.
Experiments show that SIGTERM actually does induce the Python interpreter to shut down non-daemonic threads, so installing a signal handler for that may not be necessary in the end.