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

Forward vectored writes #45

Merged
merged 4 commits into from
Mar 17, 2024
Merged

Forward vectored writes #45

merged 4 commits into from
Mar 17, 2024

Conversation

paolobarbolini
Copy link
Contributor

@paolobarbolini paolobarbolini commented Mar 3, 2024

Allows making use of rustls/rustls#1640.

Supersedes #25

I've also run tests with #44 and it seems to work correctly.

Closes #25
Closes #26

@paolobarbolini
Copy link
Contributor Author

Tests seem to get stuck every once in a while. It also happens on main

@djc djc requested review from quininer and ctz March 4, 2024 08:17
@djc djc mentioned this pull request Mar 5, 2024
@quininer
Copy link
Member

quininer commented Mar 6, 2024

[tests/early-data.rs:187:9] &line = Ok(
    "wshutting down SSL",
)
[tests/early-data.rs:187:9] &line = Ok(
    "CONNECTION CLOSED",
)
[tests/early-data.rs:187:9] &line = Ok(
    "Early data received:",
)
[tests/early-data.rs:187:9] &line = Ok(
    "orld!",
)
[tests/early-data.rs:187:9] &line = Ok(
    "End of early data",
)

It seems that due to write_vectored, the probability of words (world!) being split increases. we need a better way to check openssl output.

@quininer
Copy link
Member

quininer commented Mar 7, 2024

As a performance improvement that doesn't have to block 0.23 release, I'll try to rewrite this test over the weekend (using rustls instead of openssl).

src/common/mod.rs Outdated Show resolved Hide resolved
@paolobarbolini
Copy link
Contributor Author

This early data situation looks weird. Even if I remove the poll_write_vectored implementation it continues to happen. It's almost like openssl doesn't like early data that is split across multiple packets.

@paolobarbolini
Copy link
Contributor Author

As a performance improvement that doesn't have to block 0.23 release, I'll try to rewrite this test over the weekend (using rustls instead of openssl).

I've decided to try rewriting it myself. I opted for simplicity and left the TCP connection doing the bridging between the sync and async world.
I'll rebase it later in the day once I can get back to this.

@paolobarbolini paolobarbolini requested a review from quininer March 10, 2024 12:54
Copy link
Member

@quininer quininer left a comment

Choose a reason for hiding this comment

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

LGTM

@quininer
Copy link
Member

I have a branch based on tokio-rustls server, but I think it would be nice to use a sync implement. thank you.

Copy link
Member

@djc djc left a comment

Choose a reason for hiding this comment

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

LGTM!

src/client.rs Outdated Show resolved Hide resolved
tests/early-data.rs Show resolved Hide resolved
tests/utils.rs Outdated Show resolved Hide resolved
src/client.rs Outdated Show resolved Hide resolved
src/client.rs Outdated Show resolved Hide resolved
use std::io::Write;

// write early data
if let Some(mut early_data) = stream.session.early_data() {
Copy link
Member

Choose a reason for hiding this comment

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

It's a little different than what I thought it would be, but it's also good.

I actually think it should be outside the function.

@quininer quininer merged commit 3a153ac into rustls:main Mar 17, 2024
6 checks passed
@quininer
Copy link
Member

Thank you!

jbr added a commit to jbr/futures-rustls that referenced this pull request Mar 19, 2024
@cpu cpu mentioned this pull request Mar 21, 2024
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.

Override poll_write_vectored?
4 participants