Local Transport Improvements #489
Conversation
This change makes general improvements to the Local Transport. It updates the code to make it better documented and tested. In addition it introduces the new TransportTest, a JUnit 5 way of testing uniformly across all of the transport implementations.
yschimke
left a comment
There was a problem hiding this comment.
One open question about default ports for ws vs wss.
Overall, looks like a nice cleanup/modernization
|
|
||
| return UriHandler.super.buildServer(uri); | ||
| return Optional.of( | ||
| WebsocketServerTransport.create(uri.getHost(), WebsocketClientTransport.getPort(uri, 80))); |
There was a problem hiding this comment.
I believe this will cause a wss://rsocket.mycompany.com/endpoint to use 80 instead of 443
There was a problem hiding this comment.
That’s always been the case though. In this update, I’m aiming to not change any of the behavior as it exists. If we think it’s a bug, maybe opening an issue that I’ll work separately is the best way to go?
There was a problem hiding this comment.
Before it was only supporting plaintext as a server, so 80 was correct. IIRC my assumption at the time was it's easy to create a client with default SslContext etc, but servers generally require additional setup e.g. cert chains.
This change makes general improvements to the Local Transport. It updates the code to make it better documented and tested. In addition it introduces the new
TransportTest, a JUnit 5 way of testing uniformly across all of the transport implementations.