-
Notifications
You must be signed in to change notification settings - Fork 873
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
Replace Thread.abort with Thread.interrupt #4929
base: master
Are you sure you want to change the base?
Conversation
Thread.abort is defunt in modern .net versions. We need to be good about not swallowing exceptions blindly, interupted exception needs to be rethrown.
Note to self: Also look into the hash changes (top two commits) https://github.com/tsuckow/duplicati/commits/net5-split/fixups |
@@ -115,7 +115,7 @@ public void Dispose() | |||
|
|||
if (m_thread != null && m_thread.IsAlive) | |||
{ | |||
m_thread.Abort(); | |||
m_thread.Interrupt(); |
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.
Thread.Interrupt() on ends a thread when it hits a thread wait like sleep() or join(). It also throws different excepctions than Abort(). So exits will be a lot slower, or possibly wait until the thread is complete.
You indicate the exceptions aren't handled, so maybe that's not a problem. I've not reviewed all the relevant code to be sure.
However the interrupt on Stopnow interruption test intermittently failing does look related to threads management of ending. Only some of them have a cancellation token, so I feel that is related but I have not hard evidence for that.
Is cancellation token the desired long term solution with this and an intermediate step?
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.
Cancellation token would be ideal.
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.
As a test, I removed Thread.Abort code completely and have been running the StopNow() test. It fails a lot less when I did that. But still fails sometimes, I'm still investigating and you can foolow that in the forum thread.
thanks for that, it passes tests. The Mac test hangs intermittently but it just means that I need to find a work around, nothing to do with this PR. I have just pushed a release builder, I think that it would be better to commit this on a branch and create installers for people wanting to use an experimental release (I would at least). Given the low volume of contributions, keeping a dedicated branch and master in sync should not be too arduous I think. If you intend to contribute other backporting changes, how about asking the project owner for contributor rights ? |
This pull request has been mentioned on Duplicati. There might be relevant details there: https://forum.duplicati.com/t/stopnow-backup-and-cancellation-tokens/16223/19 |
Thread.abort is defunt in modern .net versions.
We need to be good about not swallowing exceptions blindly, interupted exception needs to be rethrown.
I have not tested this on master, it is cherry picked from net5-split