Refactor TransportCallbacks interface
#987
Comments
akoshelev
added a commit
to akoshelev/raw-ipa
that referenced
this issue
Mar 25, 2024
See private-attribution#987 for motivation. I had to decide whether I want to use dynamic dispatch vs clunky HTTP interfaces with another generic parameter propagated through the entire stack. I don't have a conslusive answer which way is better, both have significant downsides. Problems with DD approach that is proposed in this change: * Hard to keep `RequestHandler` trait object safe. No generics for `handle` method, use of `async_trait` etc. That removes the opportunity for some optimizations, namely using a trait to pass data down to the handler. It could be better if HTTP layer just passes the same structs it gets from HTTP layer without an extra conversion that must occur if dynamic dispatch is used. * Non zero-cost abstraction. To get data back from the handler, we have to use the same format, right now it is JSON but I doubt we can do better than binary serialization, which means more work to get the data out. * `Box<dyn Trait<....` is everywhere now. Problems with static dispatch (I will link a commit) is more code that requires a change. It is also not clear whether we can make it a zero-cost abstraction. It is mentioned in private-attribution#987 but I will reiterate it here that the reason for the intermediate layer data representation (betweeen HTTP and transport) is to support various delivery channels for IPA, that could potentially include something like CF workers. We don't seem to have an opportunity to rely on our network layer being HTTP in the long term.
akoshelev
added a commit
that referenced
this issue
Apr 5, 2024
) * Delete `TransportCallbacks` and use `RequestHandler` trait instead See #987 for motivation. I had to decide whether I want to use dynamic dispatch vs clunky HTTP interfaces with another generic parameter propagated through the entire stack. I don't have a conslusive answer which way is better, both have significant downsides. Problems with DD approach that is proposed in this change: * Hard to keep `RequestHandler` trait object safe. No generics for `handle` method, use of `async_trait` etc. That removes the opportunity for some optimizations, namely using a trait to pass data down to the handler. It could be better if HTTP layer just passes the same structs it gets from HTTP layer without an extra conversion that must occur if dynamic dispatch is used. * Non zero-cost abstraction. To get data back from the handler, we have to use the same format, right now it is JSON but I doubt we can do better than binary serialization, which means more work to get the data out. * `Box<dyn Trait<....` is everywhere now. Problems with static dispatch (I will link a commit) is more code that requires a change. It is also not clear whether we can make it a zero-cost abstraction. It is mentioned in #987 but I will reiterate it here that the reason for the intermediate layer data representation (betweeen HTTP and transport) is to support various delivery channels for IPA, that could potentially include something like CF workers. We don't seem to have an opportunity to rely on our network layer being HTTP in the long term. * Fix the memory leak inside TestApp * Fix one FIXME * Clean up code * Feedback
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
It is still worth to encapsulate the delivery method inside
Transportinterface, so messages delivered via HTTP or potentially CF workers channel have the same structure when handled by application logic. We may have a good abstraction for it already -Addrstruct used inside in-memory network.So our handler interface may look like this
HTTP transport can parse HTTP requests into
Addrstruct and application will have a unified logic to process requests based onAddr:RouteIdfieldOriginally posted by @akoshelev in #982 (comment)
The text was updated successfully, but these errors were encountered: