Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Adapter with configuration properties by connection/listening #54

Closed
lemunozm opened this issue Mar 11, 2021 · 5 comments
Closed

Adapter with configuration properties by connection/listening #54

lemunozm opened this issue Mar 11, 2021 · 5 comments
Labels
enhancement New feature or request

Comments

@lemunozm
Copy link
Owner

Add an easy way to pass configuration properties to the adapter when you perform a connect()/listen():
You could want to configure a specific connection with some extra properties.

Currently:

network.connect(transport, addr);
network.listen(transport, addr);

Expected:

network.connect(transport, addr, <config info to the adapter>)
network.listen(transport, addr, <config info to the adapter>)

Things to consider:

  • Avoid that the user could mix configurations from one adapter with a transport that doesn't belong. Is it possible to maintain this safety at compile time?
  • The adapter implementation should specify a default configuration to configure the adapter in case of no configuration.
@lemunozm lemunozm added the enhancement New feature or request label Mar 11, 2021
@RobDavenport
Copy link
Contributor

  • Avoid that the user could mix configurations from one adapter with a transport that doesn't belong. Is it possible to maintain this safety at compile time?

Would it be possible to solve this by instead modifying the network.connect to instead of receiving a transport as a parameter, use it as type parameter? This way we could do something like network.connect::(addr, TcpConfig::default()), or network.connect::(addr, my_config). With traits this should allow compile time checking for incompatibilities. Downsides is it might require quite a big rewrite of the network engine.

@lemunozm
Copy link
Owner Author

lemunozm commented Mar 18, 2021

Thanks, @RobDavenport ! Yeah, this could be a big candidate.

The TcpConfig should contain a Transport::Tcp internally associated with it (that identified the internal adapter used), and the connect() function could be something like Network::connect(impl TransportConfig, impl ToRemoteAddr).

The problem is, what happens if the listen() function and the connect() function can receive different configs?
Is it mandatory to create two different trait configs? TransportConnectConfig, TransportListenConfig. (problem 1)

Regarding the adapter API, the adapter function can handle directly the correct type statically, because the adapter knows all the types it uses. The tricky part is how to pass the config through the network engine as you say (without dynamic memory if possible!). (problem 2)

I had some ideas related to this feature here https://github.com/lemunozm/message-io/compare/ideas/transport_with_config. The idea is to use something like Transport::TcpWith(TcpConfig{}), for the cases you need extra configuration than the default. I am not sure if this idea obfuscates the Transport concept too much. Anyway, both previous problems remain too.

@lemunozm
Copy link
Owner Author

Regarding problem 2.

Because the TransportConfig must contain the transport (that identifies the adapter used). It could be done with some pinch of unsafe code to handle that transport as a "generic transport" and rebuild to the transport type that the adapter required by its type without any downside of performance using a Box (that could be the safe idea)

@lemunozm lemunozm changed the title Add configuration properties to the adapter. Adapter with configuration properties by connection/listening Apr 9, 2021
@kgraefe
Copy link
Contributor

kgraefe commented Mar 16, 2023

I made an attempt to resolve this in #141 by wrapping the configuration properties in an enum as @RobDavenport suggested. Having two separate enums for connect (TransportConnect) and listen ( TransportListen) allows an adapter to implement different properties on both methods. The adapter is determined by variant of the TransportConnect / TransportListen enum and then the enum is passed all the way down to the adapter. In the adapter we can assume the variant as the controller would not call this specific adapter otherwise.

The last bit cannot be statically checked, unfortunately. However, I think the code is simple enough to accept this? Also from the public API it actually is guaranteed as retrieving the adapter from the variant is an internal function.

I kept the original connect() and listen() methods with the standard Transport enum which maps to TransportConnect / TransportListen with default settings.

@lemunozm
Copy link
Owner Author

I really like your implementation. This feature is great to open the door to future extensions.

Thanks for your contribution to this!!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

3 participants