Context<S>, generic over IO

[MabezDev]

This is a MVP implementation of my proposal in #3 (comment).

• Update wrapper macros to support generic arg(s), not just lifetimes
• Make Context generic over the underlying IO

This does seem to work with my (not yet published) PR in rust-native-tls, and I can run some examples.

Notes

• I think the Context<S> implementation is sound, but HandshakeContext I am a lot more unsure of. Particularly the UnsafeFrom cast in the Sni Callback.
  • I think it works right now because HandshakeContext doesn't use the io field, but its definitely not correct.
  • Making HandshakeContext generic is probably the correct way to do it, but doing that also adds a lot of complications with the callbacks.
• I think the wrapper macros can be refactored to not duplicate everything for generics (I just chose the easiest option to get something working.)

I welcome any feedback :)

[jethrogb]

Why is this needed? You need a non-static stream?

[MabezDev]

More detail here: #3 (comment) but the general gist is that establish has stricter trait bounds than rust-native-tls. These stricter trait bounds are currently required because the stream is stored as dyn Any. This PR moves towards making Context generic over the stream, thus making the trait bounds for the Context dependent on the stream. i.e Context will be Send if the underlying stream is also Send.

[AdrianCX]

Having Context generic over IO is also in review: #163
That also addresses io_mut() returning Any - it will now return Option reference on underlying template.

That review also provides async support ported from:#127

@MabezDev let me know if first PR above covers what you need and if you spot anything.

[AdrianCX]

thus making the trait bounds for the Context dependent on the stream. i.e Context will be Send if the underlying stream is also Send.

Are you sure you want this?, you will need send for hyper support for example, async also benefits.

establish has stricter trait bounds than rust-native-tls

Correct, if you have an example where this becomes a problem I can try and help migrate that to some stricter bounds.

These stricter trait bounds are currently required because the stream is stored as dyn Any

They also help in general with integration with other libraries like hyper.

[MabezDev]

thus making the trait bounds for the Context dependent on the stream. i.e Context will be Send if the underlying stream is also Send.

Are you sure you want this?, you will need send for hyper support for example, async also benefits.

Yes, we definitely want the stream to be Send without it, its not very useful at all, sorry if that was not clear. However because of the more relaxed trait bounds on rust-native-tls's functions, it would be better to have Send automatically derived by the compiler.

Having Context generic over IO is also in review: #163

Thanks! It seems the first commit of that PR would do the job nicely; solves the HandshakeContext issue noted above by breaking out the required handshake fields into its own struct.

That PR has been a in review for a while. Provided the Context changes are not the ones blocking that PR would it be worth PR'ing those first separately perhaps?

[MabezDev]

Alternatively, I can port across the relevant changes and make a new PR (also take a stab and refactoring the define! macro)

[AdrianCX]

Sorry for the ninja edit.

Yes, we definitely want the stream to be Send without it, its not very useful at all, sorry if that was not clear. However because of the more relaxed trait bounds on rust-native-tls's functions, it would be better to have Send automatically derived by the compiler.

Do you have an example where changes are useful for / what breaks on your side if io is required to be send as it is right now?

[MabezDev]

Do you have an example where changes are useful for / what breaks on your side if io is required to be send as it is right now?

The main reason I want to remove the explicit bounds, is because rust-native-tls does not have them, only Read + Write bounds. There would be more friction in upstreaming if these bounds had to stay explicit.

They also help in general with integration with other libraries like hyper.

Could you explain how this helps? If hyper requires explicit trait bounds for Send that's fine, but with the updates to Context provided the stream is Send, Send will be auto-derived by the compiler and that trait bound will be satisfied automatically (Note: I just pushed a commit where I had forgetten to put the where S: Send bounds on the Context's Send impl). If the stream is not send, instead of the compiler error being in rust-mbedtls, it will now be in hyper (and the compiler should give a break down of what field causes the !Sendness, all the way back to the stream.)

[AdrianCX]

Makes sense, I'll look into adding that into my PR and just require that object is static.

[MabezDev]

Closing in favour of #163

[AdrianCX]

@MabezDev I assume this is what you wanted? #186 (dependencies are already merged)

[AdrianCX]

@MabezDev please also update here once you have the rust-native-tls PR raised or crate available somewhere. I was looking at doing that but will put that on hold for now since you've already started on it.

[AdrianCX]

@MabezDev started doing the native-tls work as well, some very WIP code if you're curious:

https://github.com/AdrianCX/rust-native-tls/tree/acruceru/rust-mbedtls

This pretty much plugs in place with tokio-native-tls (need to update some error messages for though)

[MabezDev]

Sorry for the delay in getting back to you. My attempt was based on the original PR: sfackler/rust-native-tls#142, just a straight rewrite using Arc etc instead of the boxed pointers in that PR.

Whilst my PR works, not all tests pass and I'm running low on cycles to work on this. I'll get to finishing this eventually, but perhaps you'd like to take the lead from here on out?

Happy to review the PR whenever it's ready, just ping me :)