add ServiceBuilder::boxed_clone_sync helper#804
Conversation
8bd56ed to
ac861f9
Compare
|
This is a quickie if you have a minute @seanmonstar , you have context on this from #777 |
| where | ||
| L: Layer<S> + Send + Sync + 'static, | ||
| L::Service: Service<R> + Clone + Send + Sync + 'static, | ||
| <L::Service as Service<R>>::Future: Send + Sync + 'static, |
There was a problem hiding this comment.
@jlizen Before this arrived in 0.5.3 I had written my own boxed_clone_sync() which had looser constraints, based on how BoxCloneSyncService is defined:
<L::Service as Service<R>>::Future: Send + 'static,
Note: The future doesn't need to be Sync.
I'd like to switch to use this ServiceBuilder helper, but my existing code won't compile because I'm doing a bunch of stuff with axum responses and the futures are Send (but not Sync).
I'm happy to stay with my current approach, but I thought I'd ask if there's a reason why this helper requires the Future to be Sync.
There was a problem hiding this comment.
I don't see a compelling reason to keep the tight bounds, given that the service itself doesn't put the Sync bounds on the future. Seems worth cutting a PR to loosen.
There was a problem hiding this comment.
Yes, please send a PR. Also please use the issues or discussions section for bringing up stuff like this, comments on old PRs are much less likely to be seen.
As requested in: #777
Adding the
ServiceBuilder::boxed_clone_sync()method to matchboxed_cloneandboxed().Note that in this case, I used the named,
BoxCloneSyncServiceLayer, rather than the layer fn returned byBoxedCloneSyncService::layer(). I don't really see a good reason to prefer the latter. I think the reason that other helpers use that, is historical (named layer didn't exist yet).Probably we should migrate everything over, but that seems likely to be a breaking change so I held off. Glad to cut an issue on that if you concur.
The only testing was via a doctest. This matches the other helper.