Skip to content

Conversation

DrSloth
Copy link
Contributor

@DrSloth DrSloth commented Mar 13, 2025

Hello, i have recently come across a situation where i wanted to use SO_REUSEPORT on the HttpServers.
I added options to the HttpServerBuilder that can be set to callbacks which can configure socket2::Socket instances that will then be used as listeners.

@DrSloth DrSloth requested a review from a team as a code owner March 13, 2025 16:34
@CLAassistant
Copy link

CLAassistant commented Mar 13, 2025

CLA assistant check
All committers have signed the CLA.

@lennartkloock lennartkloock changed the title Implement callbacks to configure the sockets used as listeners http: Implement callbacks to configure the sockets used as listeners Mar 13, 2025

// We have to create an std listener first because the tokio listener isn't clonable
let listener = tokio::net::TcpListener::bind(self.bind).await?.into_std()?;
let listener = {
Copy link
Member

Choose a reason for hiding this comment

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

I suspect this might not work on windows. Previously when adding windows support we found that if the listener was constructed outside of tokio it would block the eventloop even if non-blocking was set to true. We didnt investigate this further than that, but perhaps this might be a good time to understand why this behaviour was the case when using std::net::TcpListener::bind

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have run just fmt now. I haven't had this problem on windows yet at least when using the socket2 crate but my time spent in windows is not a lot.

@TroyKomodo
Copy link
Member

If you can format your code using just fmt I can run the windows tests to see if the socket2 bind with non-blocking actually works on windows environments.

@philipch07
Copy link
Member

?brawl try

@scuffle-brawl
Copy link
Contributor

scuffle-brawl bot commented Mar 14, 2025

⌛ Trying commit 04a569d with merge 59891e2...

scuffle-brawl bot added a commit that referenced this pull request Mar 14, 2025
http: Implement callbacks to configure the sockets used as listeners
Hello, i have recently come across a situation where i wanted to use SO_REUSEPORT on the HttpServers.
I added options to the HttpServerBuilder that can be set to callbacks which can configure `socket2::Socket` instances that will then be used as listeners.

Requested-by: philipch07 <[email protected]>
@scuffle-brawl
Copy link
Contributor

scuffle-brawl bot commented Mar 14, 2025

💔 Test failed - brawl-done

@philipch07
Copy link
Member

Oh I missed some files when reviewing since I'm currently on mobile.

When you get the chance, please run just workspace-hack so it will pass the hakari stage.

@DrSloth
Copy link
Contributor Author

DrSloth commented Mar 14, 2025

?brawl try

ah no permissions D:

@lennartkloock
Copy link
Member

?brawl try

@scuffle-brawl
Copy link
Contributor

scuffle-brawl bot commented Mar 14, 2025

⌛ Trying commit 8cf83ce with merge 47ac6af...

scuffle-brawl bot added a commit that referenced this pull request Mar 14, 2025
http: Implement callbacks to configure the sockets used as listeners
Hello, i have recently come across a situation where i wanted to use SO_REUSEPORT on the HttpServers.
I added options to the HttpServerBuilder that can be set to callbacks which can configure `socket2::Socket` instances that will then be used as listeners.

Requested-by: lennartkloock <[email protected]>
@scuffle-brawl
Copy link
Contributor

scuffle-brawl bot commented Mar 14, 2025

💔 Test failed - brawl-done

@lennartkloock
Copy link
Member

?brawl try

scuffle-brawl bot added a commit that referenced this pull request Mar 19, 2025
http: Implement callbacks to configure the sockets used as listeners
Hello, i have recently come across a situation where i wanted to use SO_REUSEPORT on the HttpServers.
I added options to the HttpServerBuilder that can be set to callbacks which can configure `socket2::Socket` instances that will then be used as listeners.

Requested-by: lennartkloock <[email protected]>
@scuffle-brawl
Copy link
Contributor

scuffle-brawl bot commented Mar 19, 2025

⌛ Trying commit 091d548 with merge 6359e68...

@scuffle-brawl
Copy link
Contributor

scuffle-brawl bot commented Mar 19, 2025

💔 Test failed - brawl-done

@lennartkloock lennartkloock requested a review from a team as a code owner March 19, 2025 20:38
@TroyKomodo
Copy link
Member

?brawl try

@scuffle-brawl
Copy link
Contributor

scuffle-brawl bot commented Mar 19, 2025

⌛ Trying commit f6cd705 with merge a3acc50...

scuffle-brawl bot added a commit that referenced this pull request Mar 19, 2025
http: Implement callbacks to configure the sockets used as listeners
Hello, i have recently come across a situation where i wanted to use SO_REUSEPORT on the HttpServers.
I added options to the HttpServerBuilder that can be set to callbacks which can configure `socket2::Socket` instances that will then be used as listeners.

Requested-by: TroyKomodo <[email protected]>
Copy link

codecov bot commented Mar 19, 2025

Codecov Report

Attention: Patch coverage is 35.71429% with 9 lines in your changes missing coverage. Please review.

Project coverage is 84.17%. Comparing base (dcf42f2) to head (f6cd705).
Report is 329 commits behind head on main.

Current head f6cd705 differs from pull request most recent head 08559d9

Please upload reports for the commit 08559d9 to get more accurate results.

Files with missing lines Patch % Lines
crates/http/src/server.rs 35.71% 9 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #407      +/-   ##
==========================================
- Coverage   84.23%   84.17%   -0.07%     
==========================================
  Files         215      215              
  Lines       14902    14844      -58     
==========================================
- Hits        12553    12495      -58     
  Misses       2349     2349              
Files with missing lines Coverage Δ
crates/http/src/backend/h3/mod.rs 87.50% <ø> (ø)
crates/http/src/backend/hyper/mod.rs 75.38% <ø> (ø)
crates/http/src/server.rs 92.08% <35.71%> (-6.32%) ⬇️

... and 7 files with indirect coverage changes

Components Coverage Δ
scuffle-aac 89.65% <ø> (ø)
scuffle-amf0 100.00% <ø> (ø)
scuffle-av1 98.66% <ø> (ø)
scuffle-batching 100.00% <ø> (ø)
scuffle-bootstrap 86.01% <ø> (ø)
scuffle-bytes-util 100.00% <ø> (ø)
scuffle-context 100.00% <ø> (ø)
scuffle-expgolomb 100.00% <ø> (ø)
scuffle-ffmpeg 91.13% <ø> (-0.12%) ⬇️
scuffle-flv 96.11% <ø> (ø)
scuffle-future-ext 50.00% <ø> (ø)
nutype-enum 71.87% <ø> (ø)
scuffle-h264 100.00% <ø> (ø)
scuffle-http 85.53% <35.71%> (-1.13%) ⬇️
scuffle-metrics 87.82% <ø> (ø)
postcompile 79.45% <ø> (-0.55%) ⬇️
scuffle-pprof 100.00% <ø> (ø)
scuffle-settings 93.66% <ø> (ø)
scuffle-signal 95.68% <ø> (+10.89%) ⬆️

@scuffle-brawl
Copy link
Contributor

scuffle-brawl bot commented Mar 19, 2025

🎉 Try build successful!
Completed in 15:16

Requested by: @TroyKomodo
Build commit: a3acc50 (a3acc500ad42fd9a9383c64ec181745a88d413f4)

DrSloth and others added 11 commits March 20, 2025 21:35
…loock

Change shebangs in Justfile from '/bin/bash/' to '/usr/bin/env bash'
This changes the shebangs in the Justfile from `#!/bin/bash` to `#/usr/bin/env bash` which is more portable across systems.

`/bin/bash` doesn't exist on e.g. nixOs, most BSDs, Alpine and apparently newer versions of macOs use a very old version of bash at `/bin/bash`.

Requested-by: lennartkloock <[email protected]>
Reviewed-by: lennartkloock <[email protected]>

impl std::fmt::Debug for ConfigureSocketCallback {
fn fmt(&self, f: &mut std::fmt::Formatter) -> std::fmt::Result {
write!(f, "ConfigureSocketCallback ")
Copy link
Member

Choose a reason for hiding this comment

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

extra space here

/// Callback to configure socket used for http1 and http2
#[cfg(any(feature = "http1", feature = "http2"))]
#[cfg_attr(docsrs, doc(cfg(any(feature = "http1", feature = "http2"))))]
configure_h12_sock: Option<ConfigureSocketCallback>,
Copy link
Member

Choose a reason for hiding this comment

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

can we make bon use an Into impl for this and then impl this for any Fn<...>

@TroyKomodo
Copy link
Member

@DrSloth did you still plan to do this pr?

@DrSloth
Copy link
Contributor Author

DrSloth commented Apr 28, 2025

@DrSloth did you still plan to do this pr?

Yes, i am very sorry. Some other projects came up that i need to attend right now.
Only unit tests are missing.
I am currently hunting for some error that prevents me from writing a unit test that expects failure. Binding twice to the same udp port for http3 fails with a aws-lc error.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants