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

SocketFrameHandler's WriteTimeout not applicable for async writes #1265

Open
Turnerj opened this issue Oct 14, 2022 · 3 comments
Open

SocketFrameHandler's WriteTimeout not applicable for async writes #1265

Turnerj opened this issue Oct 14, 2022 · 3 comments
Assignees
Milestone

Comments

@Turnerj
Copy link

Turnerj commented Oct 14, 2022

Within SocketFrameHandler, it specifically uses a write timeout and sets this on both the NetworkStream's WriteTimeout property (which internally sets it on the socket) as well as the frame handler's own WriteTimeout (which also sets it on the socket). The problem isn't necessarily setting it but the fact that the socket's SendTimeout, according to Microsoft's documentation, does not apply on asynchronous sends - the same ones used within the write loop of the socket handler.

There is an issue tracking better socket timeouts which would be applicable once they are designed and merged into the runtime but for now, the WriteTimeout gives at best a false sense of security. I can't see a way to patch around the behaviour without changes in the runtime.

I know you'd want a specific action here for your GitHub issues so I'd probably suggest some amount of documentation somewhere - perhaps both in-code for the SocketFrameHandler (I'm happy to contribute this) but also anywhere else you feel is appropriate.

For what its worth, because you read from the socket synchronously, you don't have the same issue with ReadTimeout/ReceiveTimeout.

@Turnerj
Copy link
Author

Turnerj commented Oct 14, 2022

I realised after posting this that SendHeader on SocketFrameHandler does do synchronous sends so it isn't entirely pointless to set, now just rather potentially inconsistent behaviour between the socket writes in the write loop and the send header behaviour.

@stebet
Copy link
Contributor

stebet commented Oct 14, 2022

Nice catch. As we're hopefully moving the project over to System.IO.Pipelines this would actually matter.

@lukebakken lukebakken self-assigned this Nov 18, 2023
@lukebakken lukebakken added this to the 7.0.0 milestone Nov 18, 2023
@lukebakken lukebakken modified the milestones: 7.0.0, 8.0.0 May 31, 2024
@lukebakken
Copy link
Contributor

Moving this to version 8 as there is no good solution for v7 of this library.

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

No branches or pull requests

3 participants