Removed Thread.Abort() calls #31
Conversation
|
Please squash the last commit with the first commit together to avoid git-history noise. |
| private Hashtable _Voices = Hashtable.Synchronized(new Hashtable(new CaseInsensitiveHashCodeProvider(), new CaseInsensitiveComparer())); | ||
| private Hashtable _Users = Hashtable.Synchronized(new Hashtable(StringComparer.OrdinalIgnoreCase)); | ||
| private Hashtable _Ops = Hashtable.Synchronized(new Hashtable(StringComparer.OrdinalIgnoreCase)); | ||
| private Hashtable _Voices = Hashtable.Synchronized(new Hashtable(StringComparer.OrdinalIgnoreCase)); |
There was a problem hiding this comment.
whitespace issue: SmartIrc4net uses 4 tabs per indention level not tabs
There was a problem hiding this comment.
I will have a look at this later and add (squash) a fixing commit.
Zwirbelbart
force-pushed
the
master
branch
3 times, most recently
from
7359e62 to
4c6de94
Compare
|
Re the linked comment, this is threading code, not socket-specific code afaics. In any case, the WebSocket implementation i have at the moment doesn't conflict with this. It uses the same threads, sockets and .net streams as now, the only difference is that the protocol is negotiated on connect via HTTP headers, and subsequent messages are formatted/parsed as per the WebSocket RFC (6455). |
|
@djkaty good point, this only refactors the threading code of the sockets, not the socket handling itself. |
| /// <summary> | ||
| /// | ||
| /// </summary> | ||
| public bool IsStopRequested { get; internal set; } |
|
The whole threading code has been pulled apart in pr 37 where socket handling is abstracted to allow multiple transports. Thread.Abort is removed from some of the code there by using stream closure to terminate cleanly. Using a bool flag won't work because stream reads are blocking. The Thread.Abort call still needs to be removed from the write thread in pr 37. Maybe the op can take a look? |
|
stream reads are non-blocking, they accept a timeout argument. not sure at what level, but they do. Or you can use the async functions and call Cancel on them |
|
Stream reads with StreamReader are blocking and do not accept a timeout. I went through this problem myself trying to find a solution on the other pr. You can call ReadLineAsync but this will require minimum .NET version to be bumped to 4.5. It also doesn't solve the problem anyway because the thread pool thread created will then block and need to be torn down uncleanly. The correct way to deal with the problem is to close the stream to signal the thread to end. |
|
Also, if you call await readlineasync, you are creating an additional thread negating the purpose of the existing read thread, and if you are doing it in a loop you are creating a new thread for every line of received data which is very inefficient. The blocking has to go somewhere. Anyway this is all irrelevant because PR 37 removes the read thread entirely and switches to an event driven model. |
Removed all calls to Thread.Abort() as this way of ending threads is bad style. I also fixed the deprecated Hashtables.