Pool Exception Handler: Typehints

[liamduckett]

Hey, first of all thanks for the library - it’s saved me many hours of work 🙂

I’ve recently had a small type checking issue (hopefully the right explanation?), when using the Http\Pool’s withExceptionHandler method (as explained in the docs)

The issue is reported by PhpStan, and is as follows:

Parameter #1 $callable of method Saloon\Http\Pool::withExceptionHandler()
expects callable(mixed, int|string, GuzzleHttp\Promise\PromiseInterface): void,
Closure(Saloon\Exceptions\Request\FatalRequestException|Saloon\Exceptions\Request\RequestException):
void given

Essentially, complaining that I’ve passed a callable with first argument FatalRequestException|RequestException when the expected type is mixed. Basically, the callable may be used with a first argument of type mixed , so declaring it as only allowing FatalRequestException|RequestException is unacceptable (and has potential to cause a type error).

I was going to suggest this may be an upstream issue from Guzzle, as this is built around around their promises/src/EachPromise.php, and their $onRejected property only declares a type of callable|null (so how could saloon know any more than mixed?). However, the other arguments for this callable are more specific (than EachPromise declares)...

/**
 * Handle Exception Callback
 *
 * @var \Closure(mixed, array-key, \GuzzleHttp\Promise\PromiseInterface): (void)|null
 */
protected ?Closure $exceptionHandler = null;

I’m a little unsure where to go from here, I don’t know anywhere near enough to confidently say (or figure out whether) this typehint could be narrowed (Exception would fit my use case, but if it can be more specific I think it should be). Alternatively, does my code need to be able to handle non Exception values? And if so do the docs need updating?

Possibly relevant? Sugggestion of using throwable in Guzzle github

[juse-less]

I opted for mixed, because in theory you can pass mixed to Promise::reject().

I thought about at least setting Throwable, but can't remember why I didn't off the top of my head.
I'd have to go through the Guzzle and Promise libraries again to double-check.

The reason for the extra arguments can be seen here: https://github.com/guzzle/promises/blob/6ea8dd08867a2a42619d65c3deb2c0fcbf81c8f8/src/EachPromise.php#L183

I won't have access to my computer for another two weeks, or so.
But I hope it was ar least somewhat helpful.

[liamduckett]

Thanks! Appreciate the quick response (especially when you're away from your computer...)

Both very helpful pointers. Would I be correct in thinking that as long as all "promise rejection" logic (at least, when dealing with Http\Pool) is solely under (saloon) library control, that we could potentially narrow it down from mixed anyway? (I'm thinking this aligns with your mention of double-checking.)

I've spent some time today looking through Guzzle code, but don't feel like I have an answer to the above yet...

Just an FYI (incase this affects how much time you spend on this) - this isn't a big deal to my project, nor is it time sensitive (at all). I am however, finding it quite interesting to look into!

Finally, to save me looking it up again: can pass mixed to PromiseInterface::reject().

[juse-less]

@liamduckett Hey.

I had quick look at Saloon, to see if I can remember.

But first, continuing on your self-reminder first, though, you can see in guzzlehttp/promises own README, that they are using an example with a string.
https://github.com/guzzle/promises/tree/2.0?tab=readme-ov-file#promise-rejection

Now onto the Saloon bit.

If you look in the Pool class, you can see that Saloon actually accepts people to provide their own PromiseInterfaces.

saloon/src/Http/Pool.php

Line 156 in c266550

$request instanceof PromiseInterface => yield $key => $request,

So, technically speaking, there are cases where people can provide their own PromiseInterface that rejects with non-Throwable.
Although, I feel like this may not be worth keeping the mixed type for. Not sure, as I can't really think of a use case in this context.
Quick example:

$connector = new MyConnector;
$connector->pool(function (MyConnector $connector): \Generator {
    for ($i = 0; $i < 5; $i++) {
        yield $i => $connector->sendAsync(new MyRequest)->reject('Fake error immediately');
    }
});

guzzlehttp/promises follows the Promises/A+ specification, which you can read about here:
https://promisesaplus.com/

Especially have a look at section 2.2.6.2, that describes callbacks must be called in the order of registration, and section 2.2.7, that describes how returned values vs thrown values should be handled.

If a PromiseInterface::$onRejected callback returns a value, it's passed into the next onRejected/otherwise callback, if that makes sense.
So if you picture a Request pipeline, a middleware could register itself for the PromiseInterface::$onRejected, and, rather than re-throwing or not returning anything, returns a value (like a string), that'll be sent into the next callable.

Simple example, but you can also picture middlewares that are out of your (or your SDK users) control:

$connector = new MyConnector;
$connector->pool(function (MyConnector $connector): \Generator {
    for ($i = 0; $i < 5; $i++) {
        yield $i => $connector->sendAsync(new MyRequest);
    }
})->withExceptionHandler(function (mixed $reason): mixed {
    return 'Some error';
})->withExceptionHandler(function (mixed $reason): void {
    // $reason === 'Some error'

    throw new \InvalidArgumentException('Please always throw Throwables!!');
})->withExceptionHandler(function (mixed $reason): void {
    // $reason === \InvalidArgumentException
});

I'm actually not sure if using the 3rd parameter to the $onRejected, the aggregate of all Requests PromiseInterface, to reject will case any of the Request PromiseInterfaces to be rejected or just canceled.

$connector = new MyConnector;
$connector->pool(function (MyConnector $connector): \Generator {
    for ($i = 0; $i < 5; $i++) {
        yield $i => $connector->sendAsync(new MyRequest);
    }
})->withExceptionHandler(function (mixed $reason, int|string $key, PromiseInterface $aggregatePromise): mixed {
    $promiseAggregate->reject('What will this do?');
})->withExceptionHandler(function (mixed $reason): void {
    // $reason === ???
});

[juse-less]

That being said, guzzlehttp/guzzle doesn't return values, or so.
It always throw Throwables. At least to my knowledge.

Saloon overall throws Throwables, as well.
But there are situations where Saloon will actually create a Response and return it instead.

saloon/src/Http/Senders/GuzzleSender.php

Line 175 in c266550

return ($exception = $response->toException()) ? throw $exception : $response;

So from that perspective, it could be narrowed to Response|Throwable.
But issues with mixed types would generally come from users of either the SDKs themselves, or the SDK developers.

[liamduckett]

Hey @juse-less - appreciate the time you've put into this!

Truthfully I'm not sure how much help I can be here 😅. My inexperience with Promises, and Library development in general make for a dangerous combination here ha.

I can absolutely see the value in narrowing the typehint (my usecase for example...) but I (naively) want to say that as long as this is possible:

...there are cases where people can provide their own PromiseInterface that rejects with non-Throwable...

Then I think the typehint should stay mixed - even if we can't think of any valid use cases - as people could be currently using it (even if for invalid use cases).