Skip to content

Conversation

@tekhedd
Copy link

@tekhedd tekhedd commented Oct 19, 2016

Minimal support for TCP timeouts, with tests.

There are several issues remaining in the code. In particular, several of the "Begin..." asynchronous I/O functions actually call the blocking Write functions on the socket, and are therefore dependent on the socket's SendTimeout if a connection is abandoned. A thread that is using timeouts with "async" could still end up blocking forever if a socket is abandoned at exactly the right time, although this is not necessariy an urgent problem: this issue is mostly applicable to flushing headers, and these fit into I/O buffers most of the time.

The timeouts as documented should time the "total time" to perform an action, for example to read the header, but this code applies the timeouts to indivual read/write operations. Therefore, the listener is still vulnerable to tarpit style DOS attacks.

Tom Surace and others added 2 commits October 14, 2016 10:53
Added support for a subset of HttpListenerTimeoutManager's timeouts, the
initial, read, and write timeouts. Added timeouts to
HttpListenerRequest.FlushInput().
@tekhedd
Copy link
Author

tekhedd commented Oct 19, 2016

Notes:

  • ResumableTimer is intended for use timing the total duration of a header read, body draining, etc. But I didn't get that far because there's a LOT of work to be done cleaning up the async I/O first.
  • The unit tests deal with timeouts, so they take a long time to run. So it goes!

@LukePulverenti
Copy link
Member

Thanks, just one thing to be aware of. I'm trying to mirror the mono http listener as much as possible while only adding features that it is missing:
https://github.com/mono/mono/tree/master/mcs/class/System/System.Net

The idea is that hopefully we eventually won't even need this anymore once they add WebSocket support. I guess if you're mirroring newer .net classes that is good because the mono version will probably later go that way, but just keep it in mind.

@tekhedd
Copy link
Author

tekhedd commented Oct 20, 2016

I had guessed standards-compatibility was a goal. Yes, I was sorely tempted to rename the timeout fields from the somewhat silly IIS-derived names they have now. :) But I was strong.

I thought I'd submit what I did so far more for comment right away. I'm actually pretty proud of the socket tests--networking code can be very tricky to get right. It was two days before I realized that "FlushInput" actually works correctly because the input stream considers itself "empty" when it has read the entire entity body.

I wonder if I should be submitting this upstream to the mono project, as the current code's failure modes are "very bad" in the face of flaky networking and capricious corporate firewalls: infinite timeouts and hung threads.

I could (and probably should) remove largely-unnecessary timer class and changes, to keep the changes minimal. The other changes, well, they're not sexy, but this provides the bare minimum required for a production environment.

Tom Surace and others added 3 commits May 15, 2017 14:45
…stener into MediaBrowser-master

Conflicts:
	SocketHttpListener/Net/ResponseStream.cs

Notes:
	git hates me
Merge upstream changes into main trunk.
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.

2 participants