Replace Thread.abort with Thread.interrupt #4929
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.
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.
Cancellation token would be ideal.
There was a problem hiding this comment.
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