Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fixed CPU / memory issue caused by infinite loop caused by ReadByte retu... #61

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

mburnell
Copy link

...rning data always when connection is dropped.

Issue #60

…eturning data always when connection is dropped.
@NiKiZe
Copy link
Contributor

NiKiZe commented Sep 20, 2013

This looks interesting, but could you recreate this pull request with the same indentation and coding style as existing code?

@smiley22 ?

@mburnell
Copy link
Author

My apologies - I was stumbling my way through first-time Git usage and didn't spot the inconsistent whitespace. I've brought it into line with the rest of the file now.

@smiley22
Copy link
Owner

Thanks! Shouldn't TcpClient or NetworkStream error out though if the connection is dropped? What actually happens if the connection is dropped and you call ReadByte on a NetworkStream? I would think it'd throw an exception..or does it just return -1 to indicate end of stream?

Can you resubmit to the experimental branch? Then we can do some testing there and then merge into master.

cheers,
smiley22

smiley22 added a commit that referenced this pull request Jan 20, 2014
addresses issues mentioned in #20, #51 and #61
smiley22 added a commit that referenced this pull request Jan 21, 2014
addresses issues mentioned in #20, #51 and #61
@mburnell
Copy link
Author

Sorry for such a slow reply. I don't know what happens with raw NetworkStream objects; if they throw exceptions, then great. The issue is that the stream may be an SslStream which (under some conditions) does NOT throw exceptions OR return -1 during a read operation... it just keeps returning non-negative integers indefinitely, leading to an infinite loop. The CheckConnection method I added interacts with the stream in a way that doesn't suffer from (what I consider to be) this bugged behaviour.

To reproduce it I had to programmatically disable my network adapter on another thread while the call was running, and the timing was reasonably sensitive (I think the timing window was significantly less than 100ms). The issue is pretty serious; we had at least one production environment encounter it, and besides being trapped in a loop and not receiving emails, it completely saturated a thread and led to rapid memory allocation / collection cycles.

You can move the changes across to your experimental branch if you want, but I've no interest in that branch; we'll continue using our own branch until an equivalent fix makes its way into master.

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.

3 participants