Batch exceptions/responses

[sagikazarmark]

With the terminology branch merged, I only see a few questions we need to clear, this is one of them.

Currently parallel requests work like this: if the client supports it, send requests parallel, if not, emulate it by sending them one by one. In the latter case exception have to be caught and stored for later processing.

The return value of the parallel request function is currently an array of Response objects. The main problem with that: it is hard to identify which response belongs to which request. In an ideal case, array indicies match. The proposal here is to return an object which can return a response for a specific request. Internally we could use SplObjectStorage to achieve that.

In exceptional cases, currently a BatchException is used. The problem is the same as earlier: we cannot pair requests to exceptions. SplObjectStorage could help in this as well.

Another limitation is that currently only TransferExceptions are allowed to be stored in the batch exception. However it is possible that a request is rejected because of some sort of incompatibility: invalid method, etc. In these cases the underlying client might throw an exception which cannot be transformed to a TransferException, rather an InvalidArgumentException. So we either allow any exceptions to be added for a request (which implement our interface of course) or we require to convert that exception to a TransferException?

And a third problem: in case of any exceptions thrown, all the requests should still be processed. However, if we throw a batch exception, we won't get the successful requests. A solution for this: remove batch exception and use a combination of batch exception and response proposed earlier. This way we get all the results for requests (either response or exception) and don't block the processing.

[dbu]

i think the only valid options are either to include successful responses in the BatchException (when you look for a request in the collection you see either a response or an exception) which sounds unexpected. or we do a BatchResponse that contains that map with either responses or exceptions that contain the response. BatchResponse could have convenience methods like allSuccessful() or getExceptions().

the unfortunate problem with BatchResponse is that we are back to requiring the user to examine a method result rather than rely on an exception being thrown. but it seems like the lesser evil to me.

exceptions in a BatchResponse should be anything that is associated with a request. if we are not associated and have a basic setup problem for example, an exception should still be thrown as its not an expected condition (whereas failed requests are something that happens).

[sagikazarmark]

the unfortunate problem with BatchResponse is that we are back to requiring the user to examine a method result rather than rely on an exception being thrown. but it seems like the lesser evil to me.

We need to have a way to make requests silently fail without blocking successful requests from returning. This is why an exception is not a good solution.

exceptions in a BatchResponse should be anything that is associated with a request.

I agree. Other convenience methods I can imagine: isFailed(request), isSuccessful(request), addFailedRequest(request, exception), addSuccessfulRequest(request, response)

What about exception type? I think we should still limit the exception to Http/Client/Exception instance

[dbu]

We need to have a way to make requests silently fail without blocking
successful requests from returning. This is why an exception is not a
good solution.

agreed. but the silently fail is the downside we get with this. but i
see no good way around this. its the least astonishement if we say batch
requests will always return batch results and you need to examine the
results to know what happened.

exceptions in a BatchResponse should be anything that is associated
with a request.

I agree. Other convenience methods I can imagine: isFailed(request),
isSuccessful(request)

sounds good

addFailedRequest(request, exception), addSuccessfulRequest(request, response)

this makes the BatchResponse mutable which seems wrong. can't we have
the adapter collect the information and then use constructor injection?
we probably would need to leak the SplObject implementation detail to it
however. but i could live with that. the BatchResponse is then basically
syntactic sugar around the SplObject

What about exception type? I think we should still limit the exception
to Http/Client/Exception instance

ah yes, that definitely.

[sagikazarmark]

but the silently fail is the downside we get with this.

I am not sure I understand this. The whole point is that one failed request does not make the whole request stack failed by throwing an exception. At least this is what I call "silently fail", which is not a downside IMO.

this makes the BatchResponse mutable which seems wrong.

I agree, but we have to make sure that the batch response is correctly populated. For example accepting an SplObjectStorage which is badly populated leads to problems. I would rather have a simple API which only allows specific arguments to be added, in this case, type hinting can also help. I don't know how it could be done using constructor injection.

Possible solutions:

• Have a separate builder, which then populates the response itself
• Implement the setters (adders) in an immutable way
• Make it frozen (as in option resolver component)

[hannesvdvreken]

There are couple of ways to handle failures when doing multiple requests at the same time, and that I have seen:

• Async:
  • Use of promises is the best solution here
• Sync:
  • Throw exception when one request failed. Immediately.
  • Wait till all requests finished and throw exception which holds a list of all failed requests and all succesful responses.

Both ways of throwing exceptions (when batching requests in sync) have their use cases.

[dbu]

but the silently fail is the downside we get with this.

I am not sure I understand this.

i was just making a point. its not elegant that you have to check the
response object instead of having an exception. the alternative, as
hannes points out, is to throw an exception when anything goes wrong but
have the exception provide information about what worked and what not. i
am undecided what is better, but lean towards an exception that contains
information about successful responses as well. maybe a method that
returns a BatchResponse with only the request/response pairs that where
successful?

or could we let the user decide if they want an exception or a response?
could that make sense or is it just overcomplicating things?

immediately aborting seems a bad choice to me, as its hard to tell what
did work and what not (say several posts, some might be processed on the
server already and others still being sent...).

and not sure if we can even cover async requests here. but its a good
point, we at least should design in a way that allows to add that later.

this makes the BatchResponse mutable which seems wrong.

I agree, but we have to make sure that the batch response is correctly
populated. For example accepting an SplObjectStorage which is badly
populated leads to problems. I would rather have a simple API which only
allows specific arguments to be added, in this case, type hinting can
also help. I don't know how it could be done using constructor injection.

Possible solutions:

• Have a separate builder, which then populates the response itself
• Implement the setters (adders) in an immutable way
• Make it frozen (as in option resolver component)

whether we go with batch exception or response, we will have the same
issue. imho:

the builder seems very complicated to me.

if we go with the PSR-7 idea, the setters would return a new
BatchResponse with the additional request/response or request/exception.
but that seems like a lot of overhead to me. still maybe the clean solution.

but having a method to lock it down seems actually a good idea to me.
once its locked, adders are locked as well.

[sagikazarmark]

I think we should decide the intended behavior and how it is handled:

• Should we abort immediately or wait for all requests?
• Should we throw an exception or just return an object holding all requests and responses/exceptions?
• Should we allow the user to decide the upper behavior? (silently fail or throw an exception)

I think it is hard to decide what failure really means: Let's say you send 10 requests, one fails. Is it a failure then? Or a success? It depends on the point of view. By throwing an exception we decide that even one failed request means a failure. However by returning a response object we don't declare anything like that. It just a returned result, it's your responsibility to process it.

Actually, we can simply have the same object as an exception and a response: if the user chooses to throw an exception, then it throws the result in case of a failure, otherwise it is just returned. The API would be identical in both cases IMO. We still have to decide the default behavior though.

but having a method to lock it down seems actually a good idea to me. once its locked, adders are locked as well.

Keep in mind that for such cases we need a separate FrozenException when trying to modify the object, which is not really elegant. Also, in theory object cloning is fast in PHP, so it should not be a problem from a performance point of view.

[sagikazarmark]

I checked guzzle, it returns a Pool object holding all responses/exceptions.

Generally all clients with parallel request support does this asynchronously, since in real parallel case all requests must be executed and then the response handled. I think handling errors and successes in one returned object is easier, but I don't say it is better.

[hannesvdvreken]

I think handling errors and successes in one returned object is easier

Yes, this is something that can be implemented by any adapter, whether the client underneath has support for parallel requests or not.

[sagikazarmark]

The question is then: should it be thrown, returned or decided by the user? As said, the object can extend a Exception, but can still be returned. The API would be the same anyway.

[hannesvdvreken]

If the user wants to do something clever by doing something different when one request threw an error, it can always use non-batch requests.

[sagikazarmark]

Keep in mind, that if no failure happens, we still need some sort of container to properly return the result. Currently you can only match request/response pairs based on array indices which is a pain to correctly handle, if an error occurs. This is why we need a BatchResult object anyway.

If BatchResult and BatchException are not the same object, then we can do as @dbu suggested: return BatchResult in BatchException.

[sagikazarmark]

Pushed a possible solution to the batch_response branch