Closed StaticHex closed 3 years ago
BananaHemic
commented
3 years ago Outstanding!
The way I had been using it, close was only called on application quit, so this is definitely something that I would have missed.
Thank you for taking the time to make a proper fix. Do you want to make a PR?
StaticHex
commented
3 years ago Sure, also I apologize for the late reply the last week was super busy so I wasn't able to get back to the laptop I had this cloned onto until now.
BananaHemic
commented
3 years ago Looks great, thanks for taking the time to fix that
StaticHex
commented
3 years ago No problemo And thank you for taking the time to make such a super useful plugin
On Tue, Mar 30, 2021, 12:16 PM BananaHemic @.***> wrote:
Looks great, thanks for taking the time to fix that
— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/BananaHemic/Mumble-Unity/issues/41#issuecomment-810434949, or unsubscribe https://github.com/notifications/unsubscribe-auth/ACY36BXFZ5ORJVJZRDQHJL3TGIBO7ANCNFSM4ZXXB44A .
This was primarily occurring when trying to call the mumbleClient's Close() method without exiting the application. Also only happened once out of every 5 or 6 close operations. However generally speaking Thread.Abort() is not considered safe due to the fact there's no guarantee where/when in the thread the abort will happen. A better solution would be to use Thread.Interrupt(). I was able to fix the issue by modifying the following files to use a variable to signal the threads to shutdown and then by calling Thread.Interrupt vs. abort. Here's the source code of the files I modified
MumbleUDPConnection.cs
... unchanged using statements ... namespace Mumble { public class MumbleUdpConnection { ... unchanged var defs ... // Add this to signal thread to shutdown private bool _running; ... unchanged var defs ... ... unchanged class methods ... internal void Connect() { ... do what you were doing here ... _running = true; // set flag to true here _receiveThread = new Thread(ReceiveUDP) { IsBackground = true }; _receiveThread.Start(); } ... other class stuff ... private void ReceiveUDP() { ... keep doing what you were doing here ... // Instead of while(true) use our flag here while (_running) { ... do thread stuff here ... } // This probably isn't needed but just a fail-safe. Set up thread for next run _running = true; } ... more unchanged class stuff ... internal void Close() { _running = false; // before closing, signal thread it's time to stop // The ?. in this case is the same as the null check you were doing before; no need to change // if you don't want to. However, do change Abort to Interrupt here which will wait for the thread // to resolve _receiveThread?.Interrupt(); ... unchanged ... } ... rest of the file is unchanged ...MumbleTCPConnection.cs
... unchanged using statements ... namespace Mumble { public class MumbleTcpConnection { ... unchanged var defs ... // Add this to signal threads to shut down safely private bool _running; ... unchanged var defs ... ... unchanged class methods ... internal MumbleTcpConnection(IPEndPoint host, string hostname, UpdateOcbServerNonce updateOcbServerNonce, MumbleUdpConnection udpConnection, MumbleClient mumbleClient) { ... keep doing what you were doing here ... // Set flag to true here before starting thread _running = true; _processThread = new Thread(ProcessTcpData) { IsBackground = true }; } .. unchanged class methods ... private void ProcessTcpData() { // This thread is aborted in Close() // Change while(true) to use _running here while (_running) { ... do thread stuff here (unchanged) ... } // This probably isn't needed, fail-safe to ensure we reset for next thread _running = true; } ... unchanged class methods ... internal void Close() { // Before interrupting thread, signal shutdown _running = false; ... unchanged ... // The ?. in this case is the same as the null check you were doing before; no need to change // if you don't want to. However, do change Abort to Interrupt here which will wait for the thread // to resolve _processThread?.Interrupt(); ... unchanged ... } ... rest of file is unchanged ...