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

Allow IoBuffer to hold multiple packets #531

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

Conversation

algesten
Copy link
Owner

@algesten algesten commented Jun 18, 2024

This is a potential fix for #530.

The story here is that the openssl crate has an abstraction layer SslStream which is generic over something that implements Read/Write. str0m is push/pull which means we need an "adapter" to convert between these two methods – that adapter is called IoBuffer.

I have assumed the operation of the IoBuffer would be such that each time we push an incoming packet to it, the SslStream needs to read that packet completely. The assertion tripped in #530 seem to indicate that doesn't always happen.

This PR relaxes this assumption to allow IoBuffer to potentially hold a few unconsumed packets. Because there is no repro, this is a shot in the dark.

@OxleyS
Copy link
Contributor

OxleyS commented Jun 18, 2024

I notice the Read impl for IoBuffer asserts that the buffer is big enough to consume the entire incoming buffer. If this assertion did not trip, but nevertheless self.incoming.is_empty() was false on the next set_incoming() call, wouldn't that mean read was never called?

@algesten
Copy link
Owner Author

@OxleyS yeah that's what I think too.

Would it be possible for you to test with this branch and that log message I just added? I want to see if this happens.

@OxleyS
Copy link
Contributor

OxleyS commented Jun 18, 2024

Sure, I can't promise too much since the panic has so far been extremely rare, but I can certainly throw it up there and see what happens.

Other possibilities to consider are behavior if handle_receive() was called again after some sort of error, or if the read inside OpenSSL was spuriously interrupted by a signal (the latter highly unlikely, since IoBuffer is just a buffer of bytes).

@OxleyS
Copy link
Contributor

OxleyS commented Jun 24, 2024

A small update on this - we've been running our servers with this fix since Wednesday last week, and so far haven't seen a panic nor the Data remaining in IoBuffer log line. We'll keep on running this and keeping an eye on it, though.

@algesten
Copy link
Owner Author

@OxleyS sounds promising. How frequent was it before the fix?

@OxleyS
Copy link
Contributor

OxleyS commented Jun 24, 2024

It only happened once, so it must be some rather freak conditions that trigger it.
If we haven't seen a panic nor the log line, that means those conditions have not happened yet, correct? I think we would need to see one or the other to know if this fix is working.

@algesten
Copy link
Owner Author

Yeah. Let's just give it time.

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.

None yet

2 participants