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

Asynchronous requests #29

Closed
mindplay-dk opened this issue Aug 8, 2017 · 17 comments
Closed

Asynchronous requests #29

mindplay-dk opened this issue Aug 8, 2017 · 17 comments

Comments

@mindplay-dk
Copy link

I know you explicitly wrote async HTTP clients out of the spec, and it's likely the fastest and most realistic way to get to an approved PSR within a lifetime, but I'm going to bring it up anyway, sorry.

I bring it up because a client with Promises can implement synchronous/blocking HTTP requests, but not the other way around: if we decide on an API without promises, by definition, adding support for async requests at a later time will be a breaking change.

I thought I'd mention that there is a working group who have been working on a specification of promises for PHP, although this has not been proposed as a PSR yet, as far as I know.

Essentially, without a PSR and interop for Promises, we can't really have this feature, so maybe consider getting in touch with this working group and help them push for this PSR? It has gone through four major revisions over a year, so perhaps they're not terribly far from being able to make a proposal.

Otherwise, one would have to expect already now, when deciding to use the HTTP client PSR, that the PSR will be superseded by an incompatible future PSR, which for me, personally, might be enough to just say, well, Guzzle is basically the de-facto standard anyhow, I'll just depend on that and get async support.

If I was to implement a web-scraper right now, for example, and it has to first download an HTML file, then block and wait to request and download 50 individual images/scripts/etc this is already a use-case where this PSR is basically designed for poor performance, which might already drive the decision to just keep using what we're using already.

To put it bluntly, are we going to succeed with a PSR that is inferior by design to pre-existing libraries that are already widely adopted and deployed?

On a related note, the meta document mentions Guzzle - likely the most widely-used HTTP client around. Presumably the hope is that frameworks and libraries will implement these interface, and that this leads to interoperable client implementations - but is that very interesting or useful if we don't reference the most popular existing client implementations for features?

If the abstraction reduces the leading implementation (the only implementation that's even mentioned in the meta document) to a simple blocking client, that essentially limits interop to use-cases for very basic API clients and make it uninteresting for other use-cases, such as scrapers or any other clients that need to download multiple assets in bulk.

I mean, as long as you make these decisions willingly, I suppose that's fine, but, personally, I'm already having a hard time seeing when or why I would prefer this abstraction over a specific implementation.

Maybe that's just me? :-)

@Nyholm
Copy link
Member

Nyholm commented Aug 8, 2017

I would LOVE to do async support. But I do not think we can because:

  • a PSR for HTTP clients should not define Promises
  • waiting for a Promise PSR will postpone this PSR with years(!)

The php-http team has followed discussions in async-interop closely. They could not agree with a good standard that fitted their implementations. So that organization has sadly stalled.

if we decide on an API without promises, by definition, adding support for async requests at a later time will be a breaking change.

Not necessarily. In php-http we have two interfaces, one for sync clients and one for async clients.

@sagikazarmark
Copy link
Member

Yeah, this is a pain point. When we first started to talk about submitting this PSR we decided to submit only the sync one, and later an async one when a promise PSR is worked out.

I don't think it has to be superseed by the new one though. One could always decide which interface is necessary, event clients can decide not to implement one or other.

@mindplay-dk
Copy link
Author

@Nyholm agree on both points.

@sagikazarmark true, one doesn't need to supersede the other, they can coexist - although there would likely be little or no reason to use the synchronous API if we had an async one; the async API signature works just as well for synchronous clients, but not the other way around, so an async API is more universal and should/would most likely displace this one. In any case, you're right, the interfaces can coexist.

Perhaps add a note to the meta with these points, and close this issue?

@Nyholm
Copy link
Member

Nyholm commented Aug 10, 2017

Perhaps add a note to the meta with these points, and close this issue?

Yes, thank you

@sagikazarmark
Copy link
Member

although there would likely be little or no reason to use the synchronous API if we had an async one

Well, I don't argue with that point. But I think it's already a huge step forward to have a standard for a sync API.

@kelunik
Copy link

kelunik commented Aug 10, 2017

@mindplay-dk An async client should definitely have a separate interface. It doesn't make sense to mix async and sync clients, even if every async client could be used as a sync client, it just adds unnecessary boilerplate if everything uses a async client.

As for async-interop/promise: It's unlikely React will implement this interface, they don't see a need for it, as they're currently the major package. We (amphp) on the other hand won't implement React's PromiseInterface / ExtendedPromiseInterface, as they're so bloated with functions nobody needs when working with coroutines instead of callbacks.

@dbu
Copy link

dbu commented Aug 10, 2017

great we can agree on this.

for the future: i also feel that having a sync interface has value even when we have good async support with promises. if you write a client that does not want async, having to deal with promises makes the code more complicated. (the client implementation of course can just forward to the async methods and wait on the response, but then its in the client and not the application)

@kelunik
Copy link

kelunik commented Aug 12, 2017

@mindplay-dk Another very important point is that PSR-7 is completely incompatible with async, we'd need a new PSR there as well.

@mindplay-dk
Copy link
Author

@kelunik it's "completely incompatible", how do you figure? Guzzle already does async PSR-7 requests.

@kelunik
Copy link

kelunik commented Aug 13, 2017

@mindplay-dk Do you have documentation page or example of an async streaming response in Guzzle?

@mindplay-dk
Copy link
Author

@kelunik ah, you mean the body stream... yeah, hmm... but the underlying stream is a stream resource, right? so it's read at the rate that you choose to read it - it's not necessarily an in-memory or temp-file based stream, it could be just reading from the underlying incoming stream on demand? I'm not sure how promises would change that situation.

@kelunik
Copy link

kelunik commented Aug 14, 2017

@mindplay-dk Correct, it's the streams that are problematic, not the RequestInterface or ResponseInterface. What Guzzle currently does as far as I can see is it downloads the full body to a temp:// stream and only resolves the promise once the full response is received. The complete stream is then either in memory (if it's small enough) or on the local disk. Memory access is fast enough, so let's ignore that, but if it's written do disk, the reads will just block, not for a long time, but still.

Promises or any other completion notification mechanism allow not to block, but instead just notify the caller once the result is ready. If getContents returned a promise, it could just listen to readable events on the stream, read the data in that case, and complete the getContents operation once the full stream has been received. In the meantime, there can happen other events, such as a second getContents operation on another response.

For read it can be "solved" with polling, which is pretty inefficient, but works. For getContents and __toString there's no other possibility than blocking.

@WyriHaximus
Copy link

As for async-interop/promise: It's unlikely React will implement this interface, they don't see a need for it, as they're currently the major package. We (amphp) on the other hand won't implement React's PromiseInterface / ExtendedPromiseInterface, as they're so bloated with functions nobody needs when working with coroutines instead of callbacks.

Well actually there is a PR with an implementation of async-interop/promise available at reactphp/promise#78 which stalled after you (amphp) declared the attempted failed because we (ReactPHP) weren't moving fast enough to your liking after declaring we wouldn't implement the event loop interface in its current design. Progress didn't stopped because don't see a need for it, it stopped because you pulled the plug on it. We can deal with a promises PSR in a really simple way that is to use this as implemented by ReactPHP, Guzzle, and all over JavaScript land:

interface PromiseInterface
{
    public function then(callable $onSuccess, callable $onFailure);
}

As for PromiseInterface / ExtendedPromiseInterface you don't need to implement those methods, you can just wrap then as ReactPHP does already for foreign promises and turn it into a coroutine as RecoilPHP does.

@dbu
Copy link

dbu commented Aug 15, 2017

lets please not discuss here in the http client comitee why things stalled with the promises PSR. it would be great to have that PSR, but thats the topic for the promise comitee.

i think we agree that even if the promises PSR would already exist, there is value in two separate interfaces for sync and async requests as the handling on client side is different. when async is implemented, the sync contract can basically be respected by a very thin layer that just waits on the promise.

so imho we should focus on a sync PSR, and whenver the promise PSR has reached a consensus (ideally before its released) we can start a separate PSR for async clients. that could also help to validate on a concrete use case that the promise PSR is understandable and useful. but that is another story.

@WyriHaximus
Copy link

A different PSR for sync and async clients is the way to go IMHO 👍

@kelunik
Copy link

kelunik commented Aug 16, 2017

I guess we can close this issue, as we mostly agree that async should / must have its own interface?

@Nyholm
Copy link
Member

Nyholm commented Aug 16, 2017

Yes

@Nyholm Nyholm closed this as completed Aug 16, 2017
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 a pull request may close this issue.

6 participants