bindings: match tcp EOF behavior

[lrstewart]

Description of changes:

A customer pointed out that s2n-tls's stream returns a different error kind than a TCP stream does when encountering an unexpected EOF. It seems desirable to match the TCP behavior, and in this case our ErrorType::ConnectionClosed certainly seems equivalent to std::io::ErrorKind::UnexpectedEof. I updated the error translation.

Call-outs:

This is a minor breaking change, since we're changing the io error kind we return. But the previous kind was just std::io::ErrorKind::Other, and I do think this improvement is worth it. The wrapped s2n-tls error remains unchanged.

Testing:

I added some tests that can cleanly check our behavior vs the raw TCP stream's behavior. It seems like a good test suite to expand on later.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

[jmayclin]

The types on this feel pretty complicated. I wonder if we could slightly shift things around to avoid some of the complexity?

Maybe instead of trying to pass the tcp stream back from this fixture constructor, you could just add another test case explicitly for the TCP streams? And then directly construct the streams in each test case?

Also having a generic method to assert on the read_from_closed or write_to_closed behaviors might be a nice extra check that both tcp, tls13, and tls12 all share the same behaviors.

[lrstewart]

I'm pretty attached to the idea of an array of things that should behave the same way though. If we have different test cases for TCP streams vs TLS streams, then they could end up testing for different behaviors / allowing differences. There isn't an enforcement mechanism that the behavior is the same, which is the point of this test suite.

Like, I'd love us to expand these tests to cover checking that behaviors other than handling EOF are the same between the streams.

I wonder if I could add TLS1.2 as a third element in the array tho 🤔

[lrstewart]

And while the implementation is pretty complex, calling it is pretty simple, so I'm not super worried about the complexity.

[jmayclin]

If we have different test cases for TCP streams vs TLS streams, then they could end up testing for different behaviors / allowing differences.

My thinking was that you could enforce that through an assert method.

fn assert_write_to_closed_behavior<S>(reader: S, writer: S) 
where
    S: AsyncRead + AsyncWrite // we can use a generic here to avoid relying on TLS specifics
{
    drop(writer);
    let result = reader.read_u8().await;
    assert!(result.is_err());
    let error = result.unwrap_err();
    assert!(error.kind() == std::io::ErrorKind::UnexpectedEof);
}

And then your actual unit test could be something like

fn read_from_closed_behavior() {
    let (client_tcp, server_tcp) = get_streams();
    assert_write_to_closed_behavior(client_tcp, server_tcp)
    let (client_tls13, server_tls13) = ...
    assert_write_to_closed_behavior(client_tls13, server_tls13)
}

[lrstewart]

I made this change, but idk if I like it. It moves the unique logic (like assert_write_to_closed_behavior) into a separate method while every test has to repeat the common setup logic (creating streams). It was only really reasonable when I created another helper method to abstract away tls stream creation.

It feels very inverted.

[lrstewart]

And if I want to add another kind of stream, I'm going to have to modify every test rather than just the stream setup method.

[jmayclin]

I like that the checks are now abstracted out into their own little unit of logic, but I see your concern about the stream setup. I think we can hurdle that when we come to it. There is a round-about way of defining a stream harness that takes in the "behavior assertion" closure that would get rid of the duplication, but if the streams keep increasing I agree that the dyn list approach might make more sense.

[jmayclin]

I like that the checks are now abstracted out into their own little unit of logic, but I see your concern about the stream setup. I think we can hurdle that when we come to it. There is a round-about way of defining a stream harness that takes in the "behavior assertion" closure that would get rid of the duplication, but if the streams keep increasing I agree that the dyn list approach might make more sense.