Skip to content

ServiceFn takes a closure and impls Service.#69

Closed
carllerche wants to merge 1 commit intomasterfrom
service-fn
Closed

ServiceFn takes a closure and impls Service.#69
carllerche wants to merge 1 commit intomasterfrom
service-fn

Conversation

@carllerche
Copy link
Copy Markdown
Member

ServiceFn services are always ready. It is expected that a middleware
implements a rate limiting strategy.

`ServiceFn` services are always ready. It is expected that a middleware
implements a rate limiting strategy.
@carllerche
Copy link
Copy Markdown
Member Author

I'm wondering if this is not a great API.... or at least there needs to be another one that takes Fn and wraps in an Arc.

@seanmonstar
Copy link
Copy Markdown
Collaborator

What's the proposed Arc for? You mean for making a NewService of this?

@carllerche
Copy link
Copy Markdown
Member Author

I'm wondering if ServiceFn should always take T: Fn (instead of FnMut) and stash it in an Arc. Then ServiceFn can impl NewService as well.

Basically, ServiceFn is for convenience.

@seanmonstar
Copy link
Copy Markdown
Collaborator

I thought a lot about this for hyper's local service_fn helpers. I think it probably does make sense to just take Fn, since otherwise, it would be easy for a user to think that mutating captured environment in the closure is noticeable globally, but it'd only be per-service (connection).

In the upcoming Rust release, closures will derive Copy/Clone automatically, which could mean an Arc isn't needed... though, if the captured environment includes big values, copies might be more expensive than an Arc.

@carllerche
Copy link
Copy Markdown
Member Author

@seanmonstar it also might not be obvious why an fn isn't clone, which might lead to confusing errors.

@carllerche
Copy link
Copy Markdown
Member Author

I don't remember the conclusion of this :'(

@carllerche
Copy link
Copy Markdown
Member Author

I think there should be a type that takes Fn(...) + Sync, wrap it in an Arc and impl NewService + Service.

Any thoughts re: what that type should be?

@carllerche
Copy link
Copy Markdown
Member Author

Gone stale.

@carllerche carllerche closed this Jan 12, 2019
@carllerche carllerche deleted the service-fn branch January 28, 2019 20:00
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