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

Client::send() should return a lazy future, not an outer code #14

Closed
daniel-abramov opened this issue Jul 9, 2019 · 4 comments
Closed

Comments

@daniel-abramov
Copy link

The example of fcm-rust usage from docs shows that the one is expected to use futures::future::lazy() to prepare a future which can then be spawned, however it's hardly possible in a real backend code as the Client is not Clone, i.e. each time when the user wants to spawn a notification, the whole hyper::Client will have to be moved inside the future for just one request.

It does not make sense to clone Client though as that would probably be too expensive, so if we want to return a "just a future" which makes the job done, it would make sense to wrap the future which the Client returns into Lazy inside the implementation.

@pimeys
Copy link
Collaborator

pimeys commented Jul 11, 2019

The example is not the best, I agree. You don't need to move the client though, like is done in the example found in this repo.

Hyper master works with async/await already, the tls part is about to upgrade soonish. My idea is to upgrade this, the a2 and web-push libraries to use the new syntax so that would be the right time to update the examples too in the docs.

But no, you don't have to and you should not move the client for every request.

Also did you read this example codebase on how to use these libraries to do a consumer that sends notifications?

@daniel-abramov
Copy link
Author

The example is not the best, I agree. You don't need to move the client though, like is done in the example found in this repo.

Yes, for sure. That's how I'm currently using your library.

But if you were to write a lazy future which can be spawned by some external executor (like in the similar case from the example I referred to), then it would have required a 'static future, so the client would have to be moved in this case.

My only concern was that calling Client::send() would do some job before returning the future. I.e. there are 2 common ways how the function returning future may work: some libraries do return a future which the one has to spawn to do some useful result, others start doing something and return a future which the one has to spawn in order to complete it, in other words, the latter approach does require the runtime to exist at the moment when function (send()) is called. Not a big deal for my purposes though ;)

@pimeys
Copy link
Collaborator

pimeys commented Jul 11, 2019

So when reading my old code here, it seems the only work that's done is the creation of the builder and building a request. After that it's all futures and should be polled...

@daniel-abramov
Copy link
Author

Ok, ok. I just thought that it's akin to snapview/tokio-tungstenite#55

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

No branches or pull requests

2 participants