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

Don't clone the entire service chain on every packet #615

Open
kincaidoneil opened this issue Jan 30, 2020 · 1 comment
Open

Don't clone the entire service chain on every packet #615

kincaidoneil opened this issue Jan 30, 2020 · 1 comment

Comments

@kincaidoneil
Copy link
Member

Since calling handle_request on the service interface requires a mutable borrow, we're cloning the entire service chain on every packet to get around this.

For ILP-over-HTTP requests, it happens twice:

https://github.com/interledger-rs/interledger-rs/blob/4b491ab76ca4b73ef53e101fc2cb1a71e32f53d2/crates/interledger-http/src/server.rs#L124-L135

https://github.com/interledger-rs/interledger-rs/blob/4b491ab76ca4b73ef53e101fc2cb1a71e32f53d2/crates/interledger-http/src/server.rs#L63-L74

Here's where it happens for BTP packets:

https://github.com/interledger-rs/interledger-rs/blob/4b491ab76ca4b73ef53e101fc2cb1a71e32f53d2/crates/interledger-btp/src/service.rs#L258-L259

This probably isn't great for memory usage or performance (and is probably much worse than #469 or #554). We should consider redesigning the service interface so it doesn't require a mutable borrow, since it appears very few (if any?) services require mutation.

@gakonst
Copy link
Member

gakonst commented Jan 30, 2020

It's not the mutable borrow that makes us require the clone, it's the requirement of the closure to be Fn. If you don't clone and pass by value it'll obviously be FnOnce, which doesn't work. Alternatively, you could try adding references to work around this, but I haven't figured out how to make lifetimes work in this case. There might be a way to get it working, but do note that even the Warp docs capture the state with a clone.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants