Make the client generic over transport type #195
+110
−113
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This was a lot easier than I expected, which is why I think we should go ahead with it. It would allow us to solve #194 much more easily, and also open for other transports. While I'm fine with `dyn` in the stack, I'd really prefer to avoid boxing our futures on the critical path. I thought this would be a lot messier, but it turns out that the `Session` doesn't actually need the connector at all, only the event loop does. This means that we can make `SessionEventLoop` generic over connector type, but keep `Session` completely generic. This is acceptable to me, since I expect most users just `spawn` the event loop and never pass it around manually. (If they do they'll have to make methods generic over `Connector`, probably, like I had to in the tests). Note that none of the sample applications had any breakage due to this. This _technically_ already allows for external transport implementations, but it's pretty complicated, so if we're going to open up for that we probably want some layers of abstraction to make it easier to implement on the core parts of the transport. For #194 I was considering a `TcpStreamTransport<R: AsyncRead, W: AsyncWrite>`, or something along those lines, to replace the current `TcpTransport`.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This was a lot easier than I expected, which is why I think we should go ahead with it. It would allow us to solve #194 much more easily, and also open for other transports.
While I'm fine with
dynin the stack, I'd really prefer to avoid boxing our futures on the critical path. An alternative to this would be to boxTransport. I thought using generics would be a lot messier, but it turns out that theSessiondoesn't actually need the connector at all, only the event loop does. This means that we can makeSessionEventLoopgeneric over connector type, but keepSessioncompletely without generics.This is acceptable to me, since I expect most users just
spawnthe event loop and never pass it around manually. (If they do they'll have to make methods generic overConnector, probably, like I had to in the tests). Note that none of the sample applications had any breakage due to this.This technically already allows for external transport implementations, but it's pretty complicated, so if we're going to open up for that we probably want some layers of abstraction to make it easier to implement on the core parts of the transport. For #194 I was considering a
TcpStreamTransport<R: AsyncRead, W: AsyncWrite>, or something along those lines, to replace the currentTcpTransport.There's also a separate commit with clippy lints for 1.93.