Skip to content

Removed Thread.Abort() calls#31

Open
Zwirbelbart wants to merge 1 commit into
meebey:masterfrom
Zwirbelbart:master
Open

Removed Thread.Abort() calls#31
Zwirbelbart wants to merge 1 commit into
meebey:masterfrom
Zwirbelbart:master

Conversation

@Zwirbelbart

Copy link
Copy Markdown

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

@knocte

knocte commented Apr 29, 2015

Copy link
Copy Markdown

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

Comment thread src/IrcClient/Channel.cs Outdated

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

whitespace issue: SmartIrc4net uses 4 tabs per indention level not tabs

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will have a look at this later and add (squash) a fixing commit.

@Zwirbelbart
Zwirbelbart force-pushed the master branch 3 times, most recently from 7359e62 to 4c6de94 Compare May 1, 2015 19:02
@djkaty

djkaty commented Jul 5, 2015

Copy link
Copy Markdown

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

meebey commented Jul 5, 2015

Copy link
Copy Markdown
Owner

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

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should be a volatile variable

@djkaty

djkaty commented Jul 27, 2015

Copy link
Copy Markdown

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

ghost commented Jul 27, 2015

Copy link
Copy Markdown

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

djkaty commented Jul 27, 2015

Copy link
Copy Markdown

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

djkaty commented Jul 27, 2015

Copy link
Copy Markdown

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants