Skip to content

Add close function that allows for cleaning closing tls connection #5

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

Merged
merged 2 commits into from
Mar 6, 2025

Conversation

jsturtevant
Copy link
Collaborator

When adding a "read to end of stream" type test. Needed a way to close the tls connection in a clean manner.

@jsturtevant jsturtevant force-pushed the add-close-notify-function branch from 24e3a41 to da844ea Compare February 19, 2025 23:51
Signed-off-by: James Sturtevant <jstur@microsoft.com>
@jsturtevant
Copy link
Collaborator Author

@badeend Does this look ok or should we consider a different name?

@badeend
Copy link
Collaborator

badeend commented Mar 2, 2025

I realize this signature is probably my own creation 🙃, but after looking at it with a fresh set of eyes, the return type & semantics seem a bit convoluted to me. How about stripping the return value entirely? I.e.:

close-notify: func();

Like before, this then only initiates a graceful shutdown and the completion still has to be awaited through the associated output-stream. The difference is that with this signature the closure result (successfully closed / io error) has to be obtained through the output stream as well. I've prototyped this change over at: jsturtevant/wasmtime#1. Overall this yields a net reduction in API surface area and implementation complexity, because output-stream already had all the machinery needed for this.

As added bonus, this more closely matches the behavior of tcp-socket::shutdown


should we consider a different name?

We could, yes. The reason for the current name is because I unimaginatively copied rustls' terminology, which in turn took it from the TLS spec. Do you have any suggestions?

Edit: How about close_output? To highlight the relationship between this method and the output-stream even further.

@jsturtevant
Copy link
Collaborator Author

Overall this yields a net reduction in API surface area and implementation complexity, because output-stream already had all the machinery needed for this.

As added bonus, this more closely matches the behavior of tcp-socket::shutdown

This looks like a good change to me. Matching the other api's behavior is a great benefit.

We could, yes. The reason for the current name is because I unimaginatively copied rustls' terminology, which in turn took it from the TLS spec. Do you have any suggestions?

Edit: How about close_output? To highlight the relationship between this method and the output-stream even further.

For another datapoint, I took a look at the native-ssl and they use the term shutdown (https://docs.rs/native-tls/latest/native_tls/struct.TlsStream.html#method.shutdown) which maps to the openssl terminology.

That said, I think close or close_output works in this case for spanning across implementations.

Signed-off-by: James Sturtevant <jstur@microsoft.com>
@jsturtevant jsturtevant requested a review from badeend March 4, 2025 20:43
@badeend
Copy link
Collaborator

badeend commented Mar 6, 2025

The method exists on the *-connection resource. An unqualified name like close/shutdown could lead to suggest the entire connection will be closed / shut down. Yet, this method only shuts down the writing direction; the reading direction remains operational. So I prefer the name to retain some kind of output / write / send qualifier.
I'm deeply into bike-shedding territory here, and this entire method will be gone with P3 streams anyways, so let's merge this!

Thanks! 👍

@badeend badeend merged commit 95f30bd into main Mar 6, 2025
1 check passed
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