Skip to content

Make sure we catch all exceptions #35

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.

Sign up for GitHub

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

Closed
wants to merge 1 commit into from
Closed

Make sure we catch all exceptions #35

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 1 addition & 2 deletions src/HttpAsyncClientEmulator.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,6 @@

namespace Http\Client\Common;

use Http\Client\Exception;
use Http\Promise;
use Psr\Http\Message\RequestInterface;

Expand All @@ -29,7 +28,7 @@ public function sendAsyncRequest(RequestInterface $request)
{
try {
return new Promise\FulfilledPromise($this->sendRequest($request));
} catch (Exception $e) {
} catch (\Exception $e) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What about throwables?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The RejectedPromise() takes a \Exception in its constructor. That's why we should use \Exception here.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What if we modified that to take a throwable?

return new Promise\RejectedPromise($e);
}
}
Expand Down
6 changes: 4 additions & 2 deletions src/Plugin/HistoryPlugin.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,8 +40,10 @@ public function handleRequest(RequestInterface $request, callable $next, callabl
$journal->addSuccess($request, $response);

return $response;
}, function (Exception $exception) use ($request, $journal) {
$journal->addFailure($request, $exception);
}, function (\Exception $exception) use ($request, $journal) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You need to add a typecheck here and only add the exception to the journal if it is an HTTP exception.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are we happy with the Journal only accepting HTTPlug exceptions? Or should we try to find an other solution?

if ($exception instanceof Exception) {
$journal->addFailure($request, $exception);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How can we see other exceptions then?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess we can't. Or we shouldn't. The journal is only meant to HTTP related exceptions.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Other exceptions should be handled by your code or logged/"handled" by the framework (end up in 500 usually). This Journal is to record HTTP messages and potential failures of them. This is the same questions as domain/http related exceptions.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, makes sense. :)

}

throw $exception;
});
Expand Down
3 changes: 1 addition & 2 deletions src/Plugin/RetryPlugin.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,6 @@
namespace Http\Client\Common\Plugin;

use Http\Client\Common\Plugin;
use Http\Client\Exception;
use Psr\Http\Message\RequestInterface;
use Psr\Http\Message\ResponseInterface;
use Symfony\Component\OptionsResolver\OptionsResolver;
Expand Down Expand Up @@ -62,7 +61,7 @@ public function handleRequest(RequestInterface $request, callable $next, callabl
}

return $response;
}, function (Exception $exception) use ($request, $next, $first, $chainIdentifier) {
}, function (\Exception $exception) use ($request, $next, $first, $chainIdentifier) {
if (!array_key_exists($chainIdentifier, $this->retryStorage)) {
$this->retryStorage[$chainIdentifier] = 0;
}
Expand Down
3 changes: 1 addition & 2 deletions src/PluginClient.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,6 @@
namespace Http\Client\Common;

use Http\Client\Common\Exception\LoopException;
use Http\Client\Exception as HttplugException;
use Http\Client\HttpAsyncClient;
use Http\Client\HttpClient;
use Http\Promise\FulfilledPromise;
Expand Down Expand Up @@ -79,7 +78,7 @@ public function sendRequest(RequestInterface $request)
$pluginChain = $this->createPluginChain($this->plugins, function (RequestInterface $request) {
try {
return new FulfilledPromise($this->client->sendRequest($request));
} catch (HttplugException $exception) {
} catch (\Exception $exception) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is a BC break for custom plugins, right? before, their failure callback would accept only Http\Client\Exception, but now it could be called with \Exception. this will lead to a fatal error with parameter mismatch, where before it would be an exception that broke the chain of promises...

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i do think its correct to do this, but maybe we need a BC layer for this. maybe an option to the PluginClient for this new behaviour, that is off by default. version 2 can then drop the option and always catch all exceptions.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i don't think this is breaking because the rejected promise class always said that it could take things that weren't http plug exceptions

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If one have written an function HttplugException (just like we did in HistoryPlugin) it will fail with this update. But as @GrahamCampbell says, the rejected promise class always said it would be an \Exception.

So: This is a bugfix, others may also suffer from this bug. But this is no BC break.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hm, okay. but if our own classes did not adhere to what we said, we should make sure this will go into a minor version change and that the changelog is clear about it.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So: write ducumentation that explains that only HTTPlug exceptions can be thrown in the plugin. And that's it?

What if a plugin cause a runtime exception? How is that handled? Should the next plugin see it?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mergy mergy! :trollface:

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The problem is that the used RejectedPromise accepts any kind of exceptions, so if an exception is thrown, the rejected promise will call the reject callback with any exception. IMO there are two/three ways:

  • Create a RejectedPromise, which only passes http exceptions to the callback, rethrows everything else
  • When the RejectedPromise is created, make sure only HTTP exceptions are injected into the Promise (although others are possible too) and everything else is thrown immediately (this would require to modify adapter promise implementations too)
  • Handle the scenario in the plugins: accept any exception and only process http ones, throw the rest (how about no?)

Copy link
Member

@joelwurtz joelwurtz Aug 3, 2016 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Handle the scenario in the plugins: accept any exception and only process http ones, throw the rest (how about no?)

No, or i do not maintain this xD, it will just be a nightmare to handle (also accepting this, say that we expect people not respecting httplug async standard... )

Create a RejectedPromise, which only passes http exceptions to the callback, rethrows everything else
When the RejectedPromise is created, make sure only HTTP exceptions are injected into the Promise (although others are possible too) and everything else is thrown immediately (this would require to modify adapter promise implementations too)

👍 and we must do the same for the FulfilledPromise, as actually it's accept anything, where we should only allow ResponseInterface

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The PR is here php-http/httplug#116

return new RejectedPromise($exception);
}
});
Expand Down