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

unbuffered: After Closed no WriteTraffic state arrives #1895

Open
1 task done
uazu opened this issue Apr 14, 2024 · 4 comments
Open
1 task done

unbuffered: After Closed no WriteTraffic state arrives #1895

uazu opened this issue Apr 14, 2024 · 4 comments

Comments

@uazu
Copy link

uazu commented Apr 14, 2024

Checklist

  • I've searched the issue tracker for similar bugs.

Describe the bug
I've updated my pipebuf crate Rustls wrapper in crate pipebuf_rustls to add unbuffered Rustls support. However my tests which succeed in the buffered mode fail for the unbuffered mode. The failing test does the following:

  • Client sends one byte
  • Client closes connection
  • Server sends one byte
  • Server closes connection

The byte sent by the server cannot be processed through the unbuffered interface because the process_tls_records call does not give a WriteTraffic state again after Closed. Without access to WriteTraffic, I can't send the data. The buffered interface does not have this limitation.

To Reproduce
Check out pipebuf_rustls from github (https://github.com/uazu/pipebuf_rustls). Run a single test using cargo test --no-default-features --features unbuffered byte_each_way. There is an assertion failure due to 1 byte sent but 0 bytes received.

Applicable Version(s)
0.23.4 on Linux

Expected behavior
I'd expect this case to be supported. I don't know how you'd want to support it with your unbuffered API though.

Additional context
The reason I added unbuffered support to pipebuf_rustls was that this is a much better fit to the pipebuf model. I added PBufRd::data_mut to pipebuf 0.3.1 to support Rustls unbuffered mode (which requires &mut [u8] for incoming data). So I support the unbuffered effort. Just it doesn't cover all necessary cases yet.

@ctz
Copy link
Member

ctz commented Apr 15, 2024

Thanks for the report. Yes, the current API treats a half-close as a full-close. I think we should add additional ConnectionStates so the server in your example can finish its connection with:

  • ReadTraffic reads the client's close_notify
  • PeerClosed informs the application that no further data will be forthcoming from peer
  • WriteTraffic for additional server-sent traffic, followed by queue_close_notify
  • EncodeTlsData for outputting that data
  • TransmitTlsData for confirming the data was sent
  • Closed nothing further can happen with this connection

(This arrangement retains Closed as a terminal, full-close state.)

@djc
Copy link
Member

djc commented Apr 15, 2024

Will this need semver-incompatible changes? If so, it might take us a while to get this released.

@cpu
Copy link
Member

cpu commented Apr 15, 2024

The ConnectionState enum is non_exhaustive so adding new states there isn't semver breaking in itself. I think it would be more a question of how the semantics of the overall API are updated and whether it affects existing usages.

@uazu
Copy link
Author

uazu commented Apr 15, 2024

Thanks for the quick response. There is no rush from my point of view as I also have it working with the buffered interface, and I can continue to use that. So if it's a breaking change for 0.24 that's not a problem. The API changes you suggested sound fine, but I'll code against whatever you come up with. If you want me to test against a pre-release version of Rustls I can do that.

Also it would be helpful to have some guidance about buffer-sizing requirements, either in the docs or as API calls. For example I'm allowing 18KiB for EncodeTlsData because that appears to be the maximum required by protocol, but typically it will be much less. I can add another issue for this if you wish.

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

4 participants