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

Issue #847 - add writeTimeout to jetty websocket API in jetty 10 #4344

Conversation

lachlan-roberts
Copy link
Contributor

@lachlan-roberts lachlan-roberts commented Nov 21, 2019

Signed-off-by: Lachlan Roberts [email protected]

#847

@lachlan-roberts lachlan-roberts requested a review from gregw December 4, 2019 07:45
Copy link
Contributor

@gregw gregw left a comment

Choose a reason for hiding this comment

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

Why is a write timeout needed in addition to the idle timeout, but not a read timeout? That feels asymmetric!

Is this for DOS protection? If so, then a minimum data rate might be better that a timeout because the time to write a 10GB message is obviously longer than a 1GB message, so DOS can just send lots of slow little message 1 byte at a time.

@gregw
Copy link
Contributor

gregw commented Dec 16, 2019

If this timeout is needed for the jsr API, then let's not put it in common and in other APIs. It is a wrong, non scalable configuration, like the blocking timeout that caused so many issues in HTTP.

@lachlan-roberts
Copy link
Contributor Author

@gregw this timeout is already a feature of the core API so I wanted to give it access through the jetty API as well. I don't understand why it is a "wrong, non scalable configuration".
But if you don't think it is needed in the jetty API we can close this PR.

@lachlan-roberts lachlan-roberts deleted the jetty-10.0.x-847-WebSocketPolicyWriteTimeout branch December 16, 2019 23:01
@gregw
Copy link
Contributor

gregw commented Dec 17, 2019

It doesn't scale with message size as data rate and idle timeout does. It's asymmetric as there is no read timeout. Best to leave it out of our API and look to deprecated it in core.

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