search

meebey / SmartIrc4net

IRC C# Library
http://www.meebey.net/projects/smartirc4net/
Other
126 stars 52 forks source link

Removed Thread.Abort() calls #31

Open Zwirbelbart opened 9 years ago

Zwirbelbart commented 9 years ago

Removed all calls to Thread.Abort() as this way of ending threads is bad style. I also fixed the deprecated Hashtables.

knocte commented 9 years ago

Please squash the last commit with the first commit together to avoid git-history noise.

djkaty commented 9 years ago

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).

meebey commented 9 years ago

@djkaty good point, this only refactors the threading code of the sockets, not the socket handling itself.

djkaty commented 9 years ago

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?

ghost commented 9 years ago

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

djkaty commented 9 years ago

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.

djkaty commented 9 years ago

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.