Skip to content

Conversation

@ioquatix
Copy link
Member

@ioquatix ioquatix commented Apr 22, 2024

Introduce SSLSocket#close_read and SSLSocket#close_write + relevant test.

#close_read is a no-op and exists to match the expected interface of IO.

#close_write uses #stop which in testing appears to do what we want.

Fixes #609.

@ioquatix ioquatix requested a review from rhenium April 22, 2024 08:52
@ioquatix ioquatix added this to the v3.3.0 milestone Apr 22, 2024
@rhenium
Copy link
Member

rhenium commented Apr 24, 2024

Looks good to me for TLS 1.3 connections. I'm wondering if #close_write should raise an error if it's called on a TLS 1.2 connection because it can be misleading. What do you think?

close_notify (which is sent by SSLSocket#stop = SSL_shutdown()) works differently in TLS 1.2 or older protocol versions. TLS 1.2 (https://www.rfc-editor.org/rfc/rfc5246#section-7.2.1) says:

The other party MUST respond with a close_notify alert of its own and close down the connection immediately, discarding any pending writes. It is not required for the initiator of the close to wait for the responding close_notify alert before closing the read side of the connection.

@ioquatix
Copy link
Member Author

IMHO, I think it's better just to do nothing. Raising an error will be more trouble for the user, and of little value. In other words, no other implementation of close_write can emit an error like this, so it's unexpected.

@rhenium
Copy link
Member

rhenium commented Apr 26, 2024

I don't see how #close_write could be used in a meaningful way on a TLS 1.2 connection, so users might not to want to call it at all. But I don't have a strong preference on it and I think both are OK.

Could you add some rdoc comments on the behavior difference?

@ioquatix
Copy link
Member Author

What do you think of the updated documentation?

@ioquatix
Copy link
Member Author

@rhenium thanks for your detailed review. Can you check it again?

@rhenium rhenium merged commit a7f2d22 into master Apr 30, 2024
@rhenium rhenium deleted the close-read-write branch April 30, 2024 14:51
@rhenium
Copy link
Member

rhenium commented Apr 30, 2024

Looks good to me. Thank you!

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.

Missing shutdown, close_write and close_read.

3 participants