Delete `TransportCallbacks` and use `RequestHandler` trait instead #992

akoshelev · 2024-03-25T22:41:49Z

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.

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 · 2024-03-25T22:43:36Z

e05f98e - version of RequestHandler that uses static dispatch instead.

codecov · 2024-03-25T22:53:01Z

Codecov Report

Attention: Patch coverage is 96.74797% with 20 lines in your changes are missing coverage. Please review.

Project coverage is 89.65%. Comparing base (365bb1e) to head (02ee736).

Files	Patch %	Lines
ipa-core/src/app.rs	89.06%	7 Missing ⚠️
ipa-core/src/net/transport.rs	93.10%	4 Missing ⚠️
ipa-core/src/bin/helper.rs	0.00%	2 Missing ⚠️
ipa-core/src/net/http_serde.rs	93.75%	2 Missing ⚠️
ipa-core/src/net/server/handlers/query/create.rs	90.90%	2 Missing ⚠️
ipa-core/src/helpers/transport/handler.rs	98.94%	1 Missing ⚠️
ipa-core/src/net/client/mod.rs	98.55%	1 Missing ⚠️
ipa-core/src/net/server/handlers/query/prepare.rs	96.00%	1 Missing ⚠️

Additional details and impacted files

@@           Coverage Diff            @@
##             main     #992    +/-   ##
========================================
  Coverage   89.65%   89.65%            
========================================
  Files         168      167     -1     
  Lines       22828    23012   +184     
========================================
+ Hits        20467    20632   +165     
- Misses       2361     2380    +19

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

andyleiserson · 2024-03-25T23:41:31Z

ipa-core/src/helpers/transport/handler.rs

+/// ## Performance
+/// This implementation is far from being optimal. Between HTTP and transport layer, there exists
+/// one round of serialization and deserialization to properly represent the types. It is not critical
+/// to address, because MPC helpers have to handle a constant number of requests per query. Note
+/// that all requests tagged with [`crate::helpers::transport::RouteId::Records`] are not routed
+/// through [`RequestHandler`], so there is no penalty.


I feel like this situation might improve somewhat if we stopped using axum. At somewhat increased risk of returning an invalid response, we can just give the transport layer a Vec<u8> and a content type, and have it return those without considering whether the bytes are valid for the claimed content type.

I would also like to have type safety. One of the challenges I faced before was lack of clear contract between API client and server. Here the situation is aggravated by having extra layers that don't support strong type safety. This is somewhat mitigated by e2e tests but it is easy to miss, if API is never hit.

Definitely we should have type safety (in the sense of checking requests and responses against a well-defined schema) somewhere on the datapath. But also, somewhere along the path the data has to be in a raw form. What I was trying to suggest is that we think about where the transitions are and which communication layers are dealing with raw form vs. type-safe parsed form.

let me create an issue for that

andyleiserson · 2024-03-25T23:43:10Z

ipa-core/src/helpers/transport/in_memory/transport.rs

        self: &Arc<Self>,
-        mut callbacks: L::Handler,
+        handler: Option<Box<dyn RequestHandler<Identity = I>>>,


Is there a reason this should be optional in the dynamic case, but required in the static case? (Can we not substitute the panicking handler in the cases where this would otherwise be None?)

I probably need to be consistent and either use PanickingHandler or Option. I decided that latter is better suited to indicate that there is no handler

I agree that Option probably makes more sense that PanickingHandler, although if there's just a few test cases that want to omit a handler or something like that, then maybe better to use a stub in those cases than make everybody else deal with an Option.

In any case, I don't have a strong opinion either way, I was just curious why it was different than the static case.

akoshelev · 2024-03-27T05:36:02Z

Sanitize failure was legit - so glad that we have these. I introduced a leak by having two Arc pointers pointing to each other. Resolving it was not trivial and introduced a bunch of new structs that help manage owning and non-owning ends.

andyleiserson

Looks good to me. I'm okay with either the static or dynamic version -- I think the cost of dynamic dispatch here is negligible, but the type parameters in the static version also seem within reason. Either one streamlines the transport implementations.

ipa-core/src/net/server/handlers/query/create.rs

ipa-core/src/query/executor.rs

andyleiserson · 2024-03-27T22:13:14Z

ipa-core/src/net/transport.rs

        /// Cleans up the `records_stream` collection after drop to ensure this transport
        /// can process the next query even in case of a panic.
-        struct ClearOnDrop {
+        ///
+        /// This implementation is a poor man's safety net and only works because we run
+        /// one query at a time and don't use query identifiers.


Is there an issue to improve this? Besides the issues you note, it seems like it might belong in the app or query processor rather than in (a particular implementation of) the transport layer.

I am not sure where I want to put this - it feels like transport internal state must be self-managed. We currently share the same transport instance across all queries and therefore having a need to clean up the internal state. As anything can panic at any moment, this state management is repetitive and prone to errors.

Maybe a better model would create a transport per query and then any panic will cause thread to abort, gateway and request handler to be dropped and transport destroyed.

andyleiserson · 2024-03-27T22:27:56Z

ipa-core/src/helpers/transport/handler.rs

+/// ## Performance
+/// This implementation is far from being optimal. Between HTTP and transport layer, there exists
+/// one round of serialization and deserialization to properly represent the types. It is not critical
+/// to address, because MPC helpers have to handle a constant number of requests per query. Note
+/// that all requests tagged with [`crate::helpers::transport::RouteId::Records`] are not routed
+/// through [`RequestHandler`], so there is no penalty.


Definitely we should have type safety (in the sense of checking requests and responses against a well-defined schema) somewhere on the datapath. But also, somewhere along the path the data has to be in a raw form. What I was trying to suggest is that we think about where the transitions are and which communication layers are dealing with raw form vs. type-safe parsed form.

andyleiserson · 2024-03-27T22:43:34Z

ipa-core/src/helpers/transport/in_memory/transport.rs

        self: &Arc<Self>,
-        mut callbacks: L::Handler,
+        handler: Option<Box<dyn RequestHandler<Identity = I>>>,


I agree that Option probably makes more sense that PanickingHandler, although if there's just a few test cases that want to omit a handler or something like that, then maybe better to use a stub in those cases than make everybody else deal with an Option.

In any case, I don't have a strong opinion either way, I was just curious why it was different than the static case.

akoshelev requested a review from andyleiserson March 25, 2024 22:41

andyleiserson reviewed Mar 25, 2024

View reviewed changes

Fix the memory leak inside TestApp

04a49c4

akoshelev added 2 commits March 26, 2024 22:51

Fix one FIXME

49c244a

Clean up code

facf706

andyleiserson approved these changes Mar 27, 2024

View reviewed changes

akoshelev mentioned this pull request Mar 28, 2024

Request/Response type safefy #994

Open

Feedback

02ee736

akoshelev mentioned this pull request Mar 28, 2024

Better management of IPA transport lifecycle #995

Open

akoshelev merged commit fa79cee into private-attribution:main Apr 5, 2024
11 checks passed

akoshelev deleted the transport-callbacks-die branch April 5, 2024 05:31

akoshelev mentioned this pull request Nov 4, 2024

Prepare Query API Plumbing #1398

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Delete `TransportCallbacks` and use `RequestHandler` trait instead #992

Delete `TransportCallbacks` and use `RequestHandler` trait instead #992

akoshelev commented Mar 25, 2024

akoshelev commented Mar 25, 2024

codecov bot commented Mar 25, 2024 •

edited

Loading

andyleiserson Mar 25, 2024

akoshelev Mar 27, 2024

andyleiserson Mar 27, 2024

akoshelev Mar 28, 2024

andyleiserson Mar 25, 2024

akoshelev Mar 27, 2024

andyleiserson Mar 27, 2024

akoshelev commented Mar 27, 2024

andyleiserson left a comment

andyleiserson Mar 27, 2024

akoshelev Mar 28, 2024

andyleiserson Mar 27, 2024

andyleiserson Mar 27, 2024

Delete TransportCallbacks and use RequestHandler trait instead #992

Delete TransportCallbacks and use RequestHandler trait instead #992

Conversation

akoshelev commented Mar 25, 2024

akoshelev commented Mar 25, 2024

codecov bot commented Mar 25, 2024 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

akoshelev commented Mar 27, 2024

andyleiserson left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Delete `TransportCallbacks` and use `RequestHandler` trait instead #992

Delete `TransportCallbacks` and use `RequestHandler` trait instead #992

codecov bot commented Mar 25, 2024 •

edited

Loading