-
Notifications
You must be signed in to change notification settings - Fork 704
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
Remove disabled StreamSocket::checkRemoval
criteria minimum throughput
#10325
Remove disabled StreamSocket::checkRemoval
criteria minimum throughput
#10325
Conversation
…` minimum throughput PR #9916 added `MinBytesPerSec` minimum throughput criteria for checkRemoval to limit connections, i.e. cleaning up unresponsiveness or lagging sockets. Due to its unreliable nature per request, the criteria has been disabled per default but remained in the code test w/ UT UnitTimeoutSocket. Turns out that even the UT may fail on certain test setups. We decided to remove this criteria altogether, as it will nor be used. Signed-off-by: Sven Göthel <[email protected]> Change-Id: I9212cdf8b87c1c7b0df5761db956cbaa4fd44f58
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, @sgothel. Less code is more reliable code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks sensible to me - I was really unsure why that was there in the 1st place - was this based on some research of some other server that does this.
Also - we should consider not dis-connecting sockets that still have live requests being processed - and/or/particularly those with un-sent data in the output socket-buffer.
It was one of your ideas in #9833, see #9833 (comment)
Then I shall add another PR, not hard |
Ah ! =) so slow-DOS prevention; worth reading how other proxies & web servers handle this - makes sense in the abstract of course. |
Yup, could re-add this - however, would need a well average-throughput statistical fancy working method. While the removed throughput was an average, it should have been skipped for some initial time at least I did the relaxed ping lag average on the WSPing though .. which seems to be OK. |
A threshold for open sockets is a good starting point. Only when we are above it do we need to see which sockets are slow/unresponsive and remove them. With a sensible threshold (ideally configurable), this should be safe while being defensive (when needed). |
Change-Id: I9212cdf8b87c1c7b0df5761db956cbaa4fd44f58
Summary
PR #9916 added
MinBytesPerSec
minimum throughput criteria for checkRemoval to limit connections, i.e. cleaning up unresponsiveness or lagging sockets.Due to its unreliable nature per request, the criteria has been disabled per default but remained in the code test w/ UT UnitTimeoutSocket.
Turns out that even the UT may fail on certain test setups.
We decided to remove this criteria altogether, as it will nor be used.
Checklist
make prettier-write
and formatted the code.make check
make run
and manually verified that everything looks okay