Set read buffer to 0 #20
Conversation
|
Courtesy of @mbonneau |
|
I'd like to merge this by June 8th unless any valid objections. ping @clue @WyriHaximus |
| @@ -113,6 +113,40 @@ public function testClosingStreamInDataEventShouldNotTriggerError() | |||
| $conn->handleData($stream); | |||
| } | |||
|
|
|||
| public function testBufferReadsLargeChunks() { | |||
There was a problem hiding this comment.
All other methods test the Stream in isolation by mocking the environment (unit tests), while this test uses the environment (integration test). Perhaps move this to a separate test file? (Also, there's a missing newline)
|
The code looks good to me and I'm all for getting this in! 👍 We're already handling our own buffers, so it makes sense to use unbuffered I/O on a lower level. Given how long it took to trace this (subtle) bug, we should take the time and make sure to not introduce any regressions. In particular:
|
| @@ -24,6 +24,7 @@ public function __construct($stream, LoopInterface $loop) | |||
| } | |||
|
|
|||
| stream_set_blocking($this->stream, 0); | |||
| stream_set_read_buffer($this->stream, 0); | |||
There was a problem hiding this comment.
So far this only affects reading from the stream – how about also disabling the write buffer?
There was a problem hiding this comment.
I'm leaning towards "if it ain't broke don't fix it". The read buffer is fixing a specific problem. I'm all for trying to set the write buffer to 0 in another PR if we can show it increasing performance or fixing a bug.
There was a problem hiding this comment.
if it ain't broke don't fix it
Sounds fair :)
|
@cboden What's missing for a merge? |
|
LGTM 👍 |
|
+1 |
1 similar comment
|
👍 |
We have yet to spot any regressions, so I see no point in holding this off any longer. Let's get this in 👍 |
|
Note that this PR introduced an incompatibility with HHVM which is being addressed in #28. |
refs reactphp/event-loop#26