Add the ability to define a custom request header size if you need bigger headers #217

christoph-kluge · 2017-09-07T18:56:26Z

This PR is a follow up of the discussion in #214. This allows us to configure the server with a max_header_size and pass it down to the RequestHeaderParser.

Question which I asked my self during implementation:

Do we want to fallback silently on invalid parameter usage?
Do we want to fail early on invalid parameter usage (i.e. fail on boot before the first request arrives)

jsor · 2017-09-08T07:08:00Z

Do we want to fallback silently on invalid parameter usage?

Do we want to fail early on invalid parameter usage (i.e. fail on boot before the first request arrives)

I'd say, fail early in the Server constructor.

kelunik · 2017-09-08T07:12:16Z

src/RequestHeaderParser.php

    {
        $this->localSocketUri = $localSocketUri;
        $this->remoteSocketUri = $remoteSocketUri;
+        $this->bufferMaxSize = is_integer($bufferMaxSize) ? $bufferMaxSize : 4096;


I think this should error out if it's not an integer. Just set the default value right in the parameter default.

kelunik · 2017-09-08T07:14:34Z

src/RequestHeaderParser.php


    private $localSocketUri;
    private $remoteSocketUri;

-    public function __construct($localSocketUri = null, $remoteSocketUri = null)
+    public function __construct($localSocketUri = null, $remoteSocketUri = null, $bufferMaxSize = null)


IMO it should be called $maxHeaderSize or $maxSize, the buffer might be larger than the headers are if there's a lot of data incoming at the same time.

kelunik · 2017-09-08T07:20:52Z

tests/ServerTest.php

+      $this->socket->emit('connection', array($this->connection));
+
+      $data = "GET / HTTP/1.1\r\nHost: example.com\r\nConnection: close\r\nX-DATA: ";
+      $data .= str_repeat('A', 4097 - strlen($data)) . "\r\n\r\n";


Why 4097 here? I'd expect $maxSize - strlen($data) there.

I was expecting that the default value is not throwing the overflow anymore. Your example makes more sense. I will adopt and check the other overflow tests to simply rely on those 3 test-cases:

check the default overflow $defaultValue + 1

check a custom header-size-value fits $maxSize - strlen($data)

check a custom header-size will overflow $maxSize + 1 - strlen($data)

jsor · 2017-09-08T19:01:12Z

Could fix the indention to 4 spaces to be consistent with the rest, for example here, here and in the new test methods?

jsor · 2017-09-08T22:02:19Z

There's a conflict with master now. Can you resolve it?

christoph-kluge · 2017-09-08T22:05:35Z

Just recognized it and pushed a new rebased version :)

jsor

👍

WyriHaximus · 2017-09-12T18:20:12Z

@christoph-kluge could you fix the conflicts with README.md?

christoph-kluge · 2017-09-13T12:16:53Z

@WyriHaximus done :)

WyriHaximus · 2017-09-13T12:45:11Z

@christoph-kluge great! I'll give @clue a nudge 😎

clue

@christoph-kluge Thanks for filing this high quality PR along with tests and documentation, much appreciated! 👍 I think you're addressing a very relevant issue here and I'd argue that the current default is too small anyway.

Functionally the changes LGTM, however I'll still have to vote against this for now. Let me explain:

Given the current abstractions, I think it makes perfect sense to add this as a property to the RequestHeaderParser as per this PR 👍

Unfortunately however, this class is currently hard-coded into the Server instance. To address this, this PR currently adds another indirection and introduces the concept of "options". I understand why this is done and see this as a possible solution, but I'd like to point out this violates DRY, SRP and SSOT, plus I don't see how this would evolve for future parameters ("options"). I agree that this would properly make perfect sense in a higher-level abstraction, but this library is currently quite low-level.

As an alternative, I would suggest optionally injecting the RequestHeaderParser into the Server so that only this class is responsible for holding this parameter. This will involve a slightly bigger refactoring because this class is currently stateful (this is something I've already had to start looking into on for #216).

Given that I've started working on refactoring this for #216, I would suggest completing that task first and then looking into this PR again. If you're okay with this, I may also look into cherry-picking the relevant changes once this is ready. What do you think?

Either way, thanks for filing this PR and sparking this discussion, keep it coming! 👏

christoph-kluge · 2017-09-17T23:25:38Z

@clue thanks for adding your feedback and feel free to cherry pick any changes :)

If you like I might also file another PR to support an HttpParserInterface as second parameter for the Server w/o BC breaks and then adopt this PR in order to get the option in place.

interface ReactHttpParserInterface {
  /** return a clean new instance */
  public function withConnection(Connection $conn) : self;
  public function feed($data);
}

christoph-kluge · 2017-10-13T08:40:14Z

@clue ping :) After looking up the left overs for the 0.8 milestone I found out that #40 might be co-related to this topic.

Also It's a quite an interesting area of the server as I'm also working on a "Keepalive" PR which is related to the RequestHeaderParser.

clue · 2017-10-13T08:47:43Z

@christoph-kluge Thanks for your friendly reminder :-) We are currently focusing our efforts on stabilizing a few things first and I assure you I will get back to this as soon as time permits 👍 This feature is definitely planned to be part of the upcoming v0.8.0 release

christoph-kluge · 2017-10-13T09:07:35Z

@clue thanks for you feedback! If you have more ideas/backgrounds according to that issue then I would be happy to hear. Otherwise I will try to tackle this issue again during the weekend.

…equest headers size

…cd26f227754c5deab5707c35880

…g codebase

kelunik · 2017-10-15T07:16:50Z

@clue: What's the next versions minimum requirement? Because I think there was a commit to the event loop repo that also requires 5.4.

clue · 2017-10-15T09:46:29Z

@kelunik There are no plans to change the minimum required version for this component as per the roadmap (#120).

kelunik · 2017-10-15T09:54:01Z

@clue Ok, commented on reactphp/event-loop@8997565 then. :-)

christoph-kluge · 2017-10-15T10:48:31Z

Hi @clue, I added two smaller PRs (#241 and #242) in order to discuss in separate and get stuff merged earlier. I tried to take care of #40 and #217 in order to get more close to the 0.8 milestone.

Another thing:

This will involve a slightly bigger refactoring because this class [RequestHeaderParser] is currently stateful.

I left this one open for now and would tackle this in another PR since I'm still working on the public API for that..

If I didn't missed anything I would be up for closing this PR in favor of the other created PRs.

WyriHaximus · 2017-11-14T13:43:57Z

@christoph-kluge @clue what is the status for this PR and can conflicts be fixed?

christoph-kluge · 2017-11-14T16:42:49Z

I will close this in favor of #241 and #242. Thanks for reminding @WyriHaximus 👍

WyriHaximus requested review from jsor, WyriHaximus and clue September 7, 2017 19:03

WyriHaximus added easy pick help wanted maintenance labels Sep 8, 2017

kelunik reviewed Sep 8, 2017

View reviewed changes

christoph-kluge force-pushed the custom-request-header-size branch from 29b693b to 3296b7e Compare September 8, 2017 22:04

jsor approved these changes Sep 8, 2017

View reviewed changes

WyriHaximus approved these changes Sep 9, 2017

View reviewed changes

WyriHaximus added this to the v0.8.0 milestone Sep 9, 2017

clue requested changes Sep 17, 2017

View reviewed changes

christoph-kluge added 5 commits October 14, 2017 23:32

Added option to allow a custom buffer size for the request header

6337561

Added the ability to pass "max_header_size" in order to enhance the r…

d9dd708

…equest headers size

Recovered php5.3 compatibility which was introduced by #a623a3c08d81a…

062cc6a

…cd26f227754c5deab5707c35880

Usage of max_header_size should fail early on server instantiation

8308f0f

Usage of maxSize in RequestHeaderParse should fail early

cf2ac03

christoph-kluge added 5 commits October 14, 2017 23:32

Provide php backwards compatibility for tests with exceptions

464962f

Moved indention from 2 to 4 spaces to be more consistent with existin…

f0c1169

…g codebase

Added documentation for custom options

78f0ec8

Added abstraction for a custom RequestHeaderParserInterfaces

83d3c2b

Removed options from the readme

760752e

christoph-kluge force-pushed the custom-request-header-size branch from 559ee5b to 760752e Compare October 14, 2017 21:37

christoph-kluge added 4 commits October 14, 2017 23:42

Fixed php5.3

1c6ec4e

RequestHeaderParserInterface should extend EventEmitterInterface

773f48a

Moved factory into constructor

de6df9b

Added example with custom request header size

a44569d

This was referenced Oct 15, 2017

Add max size to request header parser #241

Closed

Pass request header parser through factory #242

Closed

christoph-kluge closed this Nov 14, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add the ability to define a custom request header size if you need bigger headers #217

Add the ability to define a custom request header size if you need bigger headers #217

christoph-kluge commented Sep 7, 2017

jsor commented Sep 8, 2017

kelunik Sep 8, 2017

kelunik Sep 8, 2017

kelunik Sep 8, 2017

christoph-kluge Sep 8, 2017

jsor commented Sep 8, 2017

jsor commented Sep 8, 2017

christoph-kluge commented Sep 8, 2017

jsor left a comment

WyriHaximus commented Sep 12, 2017

christoph-kluge commented Sep 13, 2017

WyriHaximus commented Sep 13, 2017

clue left a comment

christoph-kluge commented Sep 17, 2017

christoph-kluge commented Oct 13, 2017

clue commented Oct 13, 2017

christoph-kluge commented Oct 13, 2017

kelunik commented Oct 15, 2017

clue commented Oct 15, 2017

kelunik commented Oct 15, 2017

christoph-kluge commented Oct 15, 2017

WyriHaximus commented Nov 14, 2017

christoph-kluge commented Nov 14, 2017

Add the ability to define a custom request header size if you need bigger headers #217

Add the ability to define a custom request header size if you need bigger headers #217

Conversation

christoph-kluge commented Sep 7, 2017

jsor commented Sep 8, 2017

kelunik Sep 8, 2017

Choose a reason for hiding this comment

kelunik Sep 8, 2017

Choose a reason for hiding this comment

kelunik Sep 8, 2017

Choose a reason for hiding this comment

christoph-kluge Sep 8, 2017

Choose a reason for hiding this comment

jsor commented Sep 8, 2017

jsor commented Sep 8, 2017

christoph-kluge commented Sep 8, 2017

jsor left a comment

Choose a reason for hiding this comment

WyriHaximus commented Sep 12, 2017

christoph-kluge commented Sep 13, 2017

WyriHaximus commented Sep 13, 2017

clue left a comment

Choose a reason for hiding this comment

christoph-kluge commented Sep 17, 2017

christoph-kluge commented Oct 13, 2017

clue commented Oct 13, 2017

christoph-kluge commented Oct 13, 2017

kelunik commented Oct 15, 2017

clue commented Oct 15, 2017

kelunik commented Oct 15, 2017

christoph-kluge commented Oct 15, 2017

WyriHaximus commented Nov 14, 2017

christoph-kluge commented Nov 14, 2017