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

Enforce Sync for inner services and handlers #1477

Closed
wants to merge 2 commits into from
Closed

Conversation

jplatte
Copy link
Member

@jplatte jplatte commented Oct 13, 2022

Motivation: Makes Router and RouterService Sync.

The box_clone_sync_service is copied from tower and adjusted to include Sync, none of the docs since it is private. It could probably be upstreamed into tower if we decide that we want this.

@davidpdrsn
Copy link
Member

I'm a little afraid of this since we used to require Sync in the past but reverted it because it broke other stuff (#273). At least requires a bunch of testing with real projects that uses axum to see the impact.

@jplatte
Copy link
Member Author

jplatte commented Oct 13, 2022

Yeah I agree this needs extra testing. After the coming rc.3, I can port a few things to that first, to then see whether this commit works equally well.

@davidpdrsn
Copy link
Member

I'll also test it with our stuff at Embark.

@jplatte jplatte self-assigned this Nov 5, 2022
@davidpdrsn davidpdrsn added this to the 0.7 milestone Nov 18, 2022
@davidpdrsn
Copy link
Member

Putting this on the 0.7 milestone. I'm reluctant to increase the scope of 0.6 now.

@jplatte
Copy link
Member Author

jplatte commented Nov 29, 2022

Rebased. Hope you find some time to test it, I'm probably not available for that at least for a few weeks.

@davidpdrsn davidpdrsn mentioned this pull request Mar 10, 2023
11 tasks
@davidpdrsn
Copy link
Member

davidpdrsn commented Mar 10, 2023

I've added this to the todo list for 0.7 but probably easier to redo it there rather than trying to rebase it given all the changes. I'll close this for now but still think it's something we should consider for 0.7.

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

Successfully merging this pull request may close these issues.

2 participants