Skip to content

Catch Guzzle parse request errors#65

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

Merged
WyriHaximus merged 6 commits into from
Sep 9, 2016
Merged

Catch Guzzle parse request errors #65

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
9d7896e
Catch Guzzle parse request errors and emit them as error
WyriHaximus Sep 3, 2016
398ee26
Catching exception a level higher
WyriHaximus Sep 3, 2016
4f47d79
use Exception
WyriHaximus Sep 3, 2016
dd966ba
Testing guzzle parse exception throwing
WyriHaximus Sep 3, 2016
dc0f3f3
Moved parsing and emitting to function for clarity
WyriHaximus Sep 9, 2016
80a49ed
Assert removeAllListeners
WyriHaximus Sep 9, 2016
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
14 changes: 12 additions & 2 deletions src/RequestHeaderParser.php
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -3,6 +3,7 @@
namespace React\Http;

use Evenement\EventEmitter;
use Exception;
use GuzzleHttp\Psr7 as g7;

/**
Expand All@@ -25,13 +26,22 @@ public function feed($data)
$this->buffer .= $data;

if (false !== strpos($this->buffer, "\r\n\r\n")) {
list($request, $bodyBuffer) = $this->parseRequest($this->buffer);
try {
$this->parseAndEmitRequest();
} catch (Exception $exception) {
$this->emit('error', [$exception]);

@clueclueSep 8, 2016

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Not sure this should be moved moved here, removing the listeners is not covered by any tests.

I agree that it makes sense to remove all listeners after an error, but we should probably look at line 20 then. Should this be postponed?

This whole block looks a bit unclear to me: catching exceptions from both parsing the request and emitting the headers event and continuing after an error only to remove all listeners.

@WyriHaximusWyriHaximusSep 8, 2016

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

Not sure this should be moved moved here, removing the listeners is not covered by any tests.

I'll update the tests tomorrow testing the removal of listeners.

I agree that it makes sense to remove all listeners after an error, but we should probably look at line 20 then. Should this be postponed?

Makes more sense in a separate PR tbh.

This whole block looks a bit unclear to me: catching exceptions from both parsing the request and emitting the headers event and continuing after an error only to remove all listeners.

It is controlling of the flow, if parsing the headers fails emitting will not be executed. To clear this up I'll move that to a new function instead.

}

$this->emit('headers', array($request, $bodyBuffer));
$this->removeAllListeners();
}
}

protected function parseAndEmitRequest()
{
list($request, $bodyBuffer) = $this->parseRequest($this->buffer);
$this->emit('headers', array($request, $bodyBuffer));
}

public function parseRequest($data)
{
list($headers, $bodyBuffer) = explode("\r\n\r\n", $data, 2);
Expand Down
21 changes: 21 additions & 0 deletions tests/RequestHeaderParserTest.php
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -113,6 +113,27 @@ public function testHeaderOverflowShouldEmitError()
$this->assertSame('Maximum header size of 4096 exceeded.', $error->getMessage());
}

public function testGuzzleRequestParseException()
{
$error = null;

$parser = new RequestHeaderParser();
$parser->on('headers', $this->expectCallableNever());
$parser->on('error', function ($message) use (&$error) {
$error = $message;
});

$this->assertSame(1, count($parser->listeners('headers')));
$this->assertSame(1, count($parser->listeners('error')));

$parser->feed("\r\n\r\n");

$this->assertInstanceOf('InvalidArgumentException', $error);
$this->assertSame('Invalid message', $error->getMessage());
$this->assertSame(0, count($parser->listeners('headers')));
$this->assertSame(0, count($parser->listeners('error')));

@clueclueSep 9, 2016

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I think my previous comment still holds: If we check the listeners for this error, we should probably also check the listeners for the other error event, so this provides a consistent interface for any kind of error.

@WyriHaximusWyriHaximusSep 9, 2016

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

Still agreeing with your previous comment but I rather make a new PR for that one as it feels related but out of scope for this PR

}

private function createGetRequest()
{
$data = "GET / HTTP/1.1\r\n";
Expand Down