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

Should we require Sync for services? #1950

Open
davidpdrsn opened this issue Apr 21, 2023 · 6 comments · May be fixed by #2473
Open

Should we require Sync for services? #1950

davidpdrsn opened this issue Apr 21, 2023 · 6 comments · May be fixed by #2473
Labels
A-axum I-needs-decision Issues in need of decision.
Milestone

Comments

@davidpdrsn
Copy link
Member

See #1477 for more context.

@davidpdrsn davidpdrsn added this to the 0.7 milestone Apr 21, 2023
@davidpdrsn davidpdrsn added I-needs-decision Issues in need of decision. A-axum labels Apr 21, 2023
@rakshith-ravi
Copy link
Contributor

Hey - just wanted to check in: Is this still relevant? Do we only need Sync if services should work with work-stealing executors? Otherwise, it's not needed, right?

@davidpdrsn
Copy link
Member Author

It’s still needed. Router is not Sync because it contains services that aren’t Sync. That limits our options to use things like Arc internally.

So it’s not related to work stealing.

@rakshith-ravi
Copy link
Contributor

Considering that once things are setup, we only use the router for reading (routing), what's wrong with using Arc internally? Sure, it incurs a cost of setting things up, but an initial cost shouldn't be a big deal when compared to the fact that once things are setup, when the server is running, we don't lose out on performance.

I went through the linked issue and I can't help but feel like requiring Sync would impact the kinds of services that can be used (although I'm super curious on what service was causing issues at Embark though, but I guess that's probably proprietary stuff). Anyways, what're we losing out on by using Arcs internally?

@davidpdrsn
Copy link
Member Author

Considering that once things are setup, we only use the router for reading (routing), what's wrong with using Arc internally?

Service::call takes &mut self so it’s not just for reading.

Anyways, what're we losing out on by using Arcs internally?

Not using Arc makes the router more expensive to Clone. It’s is cloned once per new connection.

@rakshith-ravi
Copy link
Contributor

Been mulling over this for a few days, and had an idea. How about we wrap everything in an Arc, and when a particular service needs to be called, we clone that particular service alone, and since we would have ownership of that service, we can call it with the &mut self.

Sorry if these suggestions seems stupid, but I'm not the most well-versed with axum internals. I'm just randomly throwing ideas in hopes of seeing if something sticks

@davidpdrsn
Copy link
Member Author

Been mulling over this for a few days, and had an idea. How about we wrap everything in an Arc, and when a particular service needs to be called, we clone that particular service alone, and since we would have ownership of that service, we can call it with the &mut self.

That won't work. For Arc<T> to be Send, T must be Sync and axum's Router and futures need to be Send.

@davidpdrsn davidpdrsn modified the milestones: 0.7, 0.8 Nov 10, 2023
@davidpdrsn davidpdrsn linked a pull request Dec 30, 2023 that will close this issue
1 task
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-axum I-needs-decision Issues in need of decision.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants