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 4 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
9 changes: 7 additions & 2 deletions src/RequestHeaderParser.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
namespace React\Http;

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

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

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

@clue clue Sep 8, 2016

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.

Copy link
Member Author

@WyriHaximus WyriHaximus Sep 8, 2016

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();
}
}
Expand Down
16 changes: 16 additions & 0 deletions tests/RequestHeaderParserTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -113,6 +113,22 @@ 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;
});

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

$this->assertInstanceOf('InvalidArgumentException', $error);
$this->assertSame('Invalid message', $error->getMessage());
}

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