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

response: We *are* blocking! #103

Merged
merged 1 commit into from
Jan 8, 2024
Merged

response: We *are* blocking! #103

merged 1 commit into from
Jan 8, 2024

Conversation

mrkline
Copy link
Contributor

@mrkline mrkline commented Jan 7, 2024

We're not using some event-driven async I/O, that's the whole point of this crate. The sockets are not set up as non-blocking and we should only expect EWOULDBLOCK from recv() on timeouts.

@mrkline
Copy link
Contributor Author

mrkline commented Jan 7, 2024

Correct me if I'm wrong - POSIX OSes (Mac and Linux) don't return EWOULDBLOCK from syscalls like recv() unless:

  1. they're configured as non-blocking with fcntl(..., O_NONBLOCK), and the same seems to be true for Windows: https://learn.microsoft.com/en-us/windows/win32/api/winsock/nf-winsock-recv
  2. They timed out (bit of a misnomer, but so it goes.)

I think we could make similar changes to code in native_tls/, let me know!

@mrkline mrkline force-pushed the blocking branch 2 times, most recently from 44a0d6d to d01756d Compare January 7, 2024 07:32
@neonmoe
Copy link
Owner

neonmoe commented Jan 7, 2024

Good point! I kinda thought that reading from the socket would be similar to waiting for a mutex or similar, where you might get "spurious wakeups" though I've never seen them. But if wouldblock only really happens on timeout with blocking sockets, it seems like a good idea to just not handle wouldblocks anywhere, except for detecting timeouts.

For timeouts though, rather than doing it in response.rs, I'd rather check for wouldblock in HttpStream's read implementation, and return Err(timeout_err()) instead of wouldblock. Response should just pass the errors to the user.

@mrkline mrkline force-pushed the blocking branch 3 times, most recently from b42aa9d to 6388514 Compare January 8, 2024 01:42
@mrkline
Copy link
Contributor Author

mrkline commented Jan 8, 2024

You're right, that's much cleaner! Pushed an updated patch.

We're not using some event-driven async I/O, that's the whole point of
this crate. Sockets are not set up as non-blocking and we should only
expect EWOULDBLOCK from recv() on timeouts.
@neonmoe neonmoe merged commit 38481c8 into neonmoe:master Jan 8, 2024
5 checks passed
@neonmoe
Copy link
Owner

neonmoe commented Jan 8, 2024

Thanks for another contribution, much appreciated!

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