Protect the TCP connection against close from other streams #133

legionth · 2017-02-23T00:29:24Z

Currently the ChunkedDecoder and the LengthLimitedStream emit errors, but these errors aren't handled anywhere.
This takes care that theses errors will be forwarded to the request object.

This PR also adds an CloseProtectionStream to protect the TCP connection against a close , when an error occurs in any stream. Instead of closing the TCP stream it will be paused, similar to the current behavior of the request object.

clue · 2017-02-27T15:48:25Z

src/Server.php

+        // Forward occuring errors on to request
+        $stream->on('error', function ($error) use ($request) {
+            $request->emit('error', array($error));
+        });


This was probably prepared before #132 was in, this should now be moved to the Request 👍

Must be slipped through in a rebase 🙈 . It was already moved to the Request object. Removed it.

clue · 2017-02-27T15:50:53Z

src/CloseProtectionStream.php

+use React\Stream\Util;
+
+/** @internal */
+class CloseProtectionStream extends EventEmitter implements ReadableStreamInterface


The class LGTM, but I suppose its purpose may be a bit unclear for outsiders ;-) Can you add some documentation here? 👍

Added a description. Have look. Tell me if I missed a point.

clue · 2017-02-27T15:52:35Z

tests/CloseProtectionStreamTest.php

+
+class CloseProtectionStreamTest extends TestCase
+{
+    public function testCloseEventDoesntCloseInputStream()


So what does it do instead? :-)

clue · 2017-02-27T15:53:05Z

tests/CloseProtectionStreamTest.php

+        $input->expects($this->once())->method('pause');
+
+        $protection = new CloseProtectionStream($input);
+        $protection->pause();


Looks like this is a duplicate of a test below?

You are right. Must be leftover of a previous local version.

@legionth ping?

clue · 2017-02-27T15:54:47Z

src/CloseProtectionStream.php

+     public function handleError(\Exception $e)
+     {
+         $this->emit('error', array($e));
+         $this->close();


Not saying this is wrong, but the close() is probably not needed currently and may be an issue with the future Stream version (in case this is a non-permanent error). I guess it's probably safer to leave the close() out here and leave this up to the input stream?

clue · 2017-02-27T15:55:42Z

tests/CloseProtectionStreamTest.php

+        $protection->on('close', $this->expectCallableOnce());
+
+        $input->close();
+        $input->emit('end', array());


This looks like an invalid sequence of events to me, what is being tested here?

Was a leftover of previous version. Removed it.

clue · 2017-02-27T15:56:20Z

tests/CloseProtectionStreamTest.php

+        $input->emit('end', array());
+
+        $this->assertFalse($protection->isReadable());
+    }


See above, is this a duplicate?

clue · 2017-02-27T15:57:55Z

src/CloseProtectionStream.php

+         $this->emit('close');
+
+         // 'pause' the stream avoids additional traffic transferred by this stream
+         $this->input->pause();


I don't see this covered, but what happens if I call this:

$protection->close(); $protection->resume();

You are right, this wasn't handled correctly. Fixed it and added some tests for it.

clue · 2017-02-27T16:09:38Z

tests/ServerTest.php

+        $server->on('request', function ($request, $response) use (&$error){
+            $request->on('error', function ($ex) use (&$error) {
+                $error = $ex;
+            });


Should this use expectCallableOnce() instead? (see also below)

I thought about this, too. But I wanted to ensure that the expected type is an Exception and nothing else. Do you think it is enough to check this behavior in ChunkedDecoderTest?

With the curren commits only expectCallableOnce will be called and not the assertion of an Exception.

I think the updated tests are easier to grasp 👍 IF the exception class is really important here (I'm undecided about this), then you may want to use expectCallableOnceWith(…) and family?

Good point! Added that in the newest commit.

clue · 2017-02-27T16:11:17Z

tests/ServerTest.php

        $this->assertInstanceOf('InvalidArgumentException', $error);
    }

+    public function testInvalidChunkHeaderResultsInErrorResponse()


Afaict this does not "result in error reponse", but actually "result in an error on the request stream"?

These tests (see below) LGTM otherwise, but I'm not sure if they provide much value in ServerTest or if we should re-organize this? Any input would be welcome 👍

Re-organize in which way?
I wanted to show that the server can handle the errors thrown by the ChunkedDecoder and the LengthLimitedStream. Do you think these tests are not necessary? Or is it the similar logic for these tests that concerns you?

Changed the test names.
I consider to reorganize it in a seprated PR. Currently the ServerTest is more an integration test than an unit test. That is completly fine but maybe this should be seperated. I would keep the tests in this PR, because this would affect several other tests too.

The updated tests now look good to me and I agree that it makes sense to re-organize this in a later follow-up PR 👍

clue

The tests could use an update, otherwise LGTM 👍

clue · 2017-02-28T12:45:12Z

tests/CloseProtectionStreamTest.php

+        $input->expects($this->once())->method('pause');
+
+        $protection = new CloseProtectionStream($input);
+        $protection->pause();


@legionth ping?

clue · 2017-02-28T12:47:54Z

tests/CloseProtectionStreamTest.php

+        $protection->close();
+    }
+
+    public function testPause()


Please check the test names (and below)

Refactored this, I hope the names are easier to understand.

clue

Nice! 👍

legionth · 2017-03-01T11:35:48Z

Just squashed the commits.

legionth · 2017-03-01T12:10:46Z

Updated the commit history and added an README update.

clue requested changes Feb 27, 2017

View reviewed changes

clue added this to the v0.6.0 milestone Feb 27, 2017

clue added the new feature label Feb 27, 2017

legionth force-pushed the error-handling branch 2 times, most recently from 0572c12 to 1318841 Compare February 28, 2017 11:57

clue requested changes Feb 28, 2017

View reviewed changes

legionth changed the title ~~Handle errors from ChunkedDecoder and LengthLimitedStream~~ Protect the TCP connection against close from other streams Feb 28, 2017

clue approved these changes Mar 1, 2017

View reviewed changes

jsor approved these changes Mar 1, 2017

View reviewed changes

legionth force-pushed the error-handling branch from e1cea09 to 833d8d8 Compare March 1, 2017 11:33

legionth added 3 commits March 1, 2017 12:51

Test error events from other streams on request object

220f00a

Protect streams against close of other streams on error

f7d45c5

Update README

9969b67

legionth force-pushed the error-handling branch from 833d8d8 to 9969b67 Compare March 1, 2017 12:09

WyriHaximus approved these changes Mar 1, 2017

View reviewed changes

WyriHaximus merged commit a445cc7 into reactphp:master Mar 1, 2017

Uh oh!

Protect the TCP connection against close from other streams #133

Protect the TCP connection against close from other streams #133

Uh oh!

Conversation

legionth commented Feb 23, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

clue left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

clue left a comment

Choose a reason for hiding this comment

Uh oh!

legionth commented Mar 1, 2017

Uh oh!

legionth commented Mar 1, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants