Opened 4 years ago

Closed 3 years ago

#6927 enhancement closed fixed (fixed)

Reduce limit of HTTP headers size

Reported by: Adi Roiban Owned by: Adi Roiban
Priority: normal Milestone:
Component: web Keywords:
Cc: jknight Branch: branches/header-limits-6927-4
branch-diff, diff-cov, branch-cov, buildbot
Author: glyph

Description

twisted.protocols.basic.LineRecever has a default line size of 16384.

twisted.web.http.HTTPChannel has a default limit of 500 header.

Doing the math this gives a maximum total header size of about 7.8MB.

Here is someone reporting some limits of various web servers: http://stackoverflow.com/a/8623061/539264

7M is more than limit of other web servers, but it not critical.

I think that single header limit of 16KB is ok as some users might want to send large cookies.

Please let me know you think that current HTTPChannel limit is ok and if not, please suggest the default value for maximum allowed size.

Attachments (4)

6927-1.diff (8.7 KB) - added by Adi Roiban 4 years ago.
6927-2.diff (12.4 KB) - added by Adi Roiban 4 years ago.
6972-3.diff (5.6 KB) - added by Adi Roiban 4 years ago.
6927-4.diff (11.2 KB) - added by Adi Roiban 3 years ago.

Download all attachments as: .zip

Change History (35)

comment:1 Changed 4 years ago by DefaultCC Plugin

Cc: jknight added

comment:2 Changed 4 years ago by lvh

The good news is that these limits already look *fairly* sane. 8kB should be enough for everyone (since that's what you'd get if you weren't using Twisted right now anyway), but it also means that 16kB is not crazy egregious.

Are there any headers that are regularly longer than 8kB? Are there headers that can be longer than cookies in normal circumstances (obviously you can make any header whatever you want)? Also, is 500 a sensible number of max headers?

comment:3 Changed 4 years ago by Adi Roiban

Keywords: review added
Owner: Adi Roiban deleted

I am putting this to review, since without feedback I can not continue working at fixing this bug.

Please let me know if you accept this ticket/bug and let me know the default limits.

I still think that the current default limit of 7.8MB is quite big.

Now, if you place nginx in front a twisted, this might not be a problem since nginx has some good configuration option and you can work around this... but in this case, maybe we should include a warning in the documentation and instruct users to not use twisted web without a proxy.

comment:4 Changed 4 years ago by lvh

Keywords: review removed
Owner: set to Adi Roiban

I'm not sure what feedback you'd like; I think my previous comment raised a few open questions :) As for acceptance, I certainly think this is a decent ticket, and the Twisted development process doesn't require fiat from anyone in particular :)

FWIW:

  • I'm +0 on changing the individual header size limit from 8kB to 16kB
  • I'm +1 on reducing the number of allowed headers to something smaller
  • I'm strongly +1 on a change that keeps track of the *total* header size instead of just a limit per header and a limit on the number of headers, since that would more accurately deal with the potential issue you're talking about, right?

However, we have to consider how the compatibility policy fits into this. I think this fits under:


Generally, the reason that one would want to do this is to give applications a performance enhancement or bug fix that could break behavior that unanticipated, hypothetical uses of an existing API, but we don't want well-behaved applications to pay the penalty of a deprecation/adopt-a-new-API/removal cycle in order to get the benefits of the improvement if they don't need to.


... but people have been known to disagree with me :)

comment:5 Changed 4 years ago by Adi Roiban

Keywords: review added
Owner: Adi Roiban deleted

Here is the ticket.

I have updated the runRequest test method, since I find it hard to always pass success=False, when that part of the method is not used. I have also added a docstring for that method. I have updated to pass 'channel' to it so that patching is not required. Hope that you will like those changes.

This patch only add a limit of 16K for total header size: headers + initial line.

The limit of 500 headers was not changed... I am even thinking of removing this limit.

The limit for a single header line is unchanged at 16K, defined by basic.LineReceiver.


With this patch I am trying to prevent using 780M of memory for 100 malicious HTTP requests... with this patch such requests should only use 1.5M,

Thanks!

Changed 4 years ago by Adi Roiban

Attachment: 6927-1.diff added

comment:6 Changed 4 years ago by Glyph

Author: glyph
Branch: branches/header-limits-6927

(In [41724]) Branching to header-limits-6927.

comment:7 Changed 4 years ago by Glyph

Hi adiroiban,

Thanks for this contribution.

First, a minor nitpick; for the time being (while we're still using SVN and submitting patches, at least) please configure your git repository as specified in the git mirror documentation to remove prefixes; this makes it a bit easier to automatically download and apply your patch.

comment:8 Changed 4 years ago by Glyph

Keywords: review removed
Owner: set to Adi Roiban

Things that need to be addressed before merging:

  1. Thanks for adding a docstring for runRequest, but unfortunately this isn't really in the format our documentation tool accepts. It looks like you've written some other epytext on this patch, so just follow that example and twistedchecker should be happy :-).
  2. New @ivar declarations should all have @type fields to go with them, explaining what type things are.
  3. Private variables should be documented too. _receivedHeaderSize should have an @ivar in the docstring as well.
  4. Is it really kosher to just splat some response bytes as a static string?
  5. Just like you should be able to customize your 500 pages, it should be possible to customize your 400 pages as well. Right now if a web browser starts sending slightly too much header data, there will be absolutely no feedback whatsoever to the user. Given that there have been header-inflation attacks against certain applications to cause exactly this behavior, it seems like a really bad failure mode, especially since we're introducing it.

Thoughts:

  1. We really need to have some kind of system for ... ugh ... configuration. Values like maxHeaders and totalHeadersSize are plausibly things that someone would want to tweak for a particular deployment without necessarily writing code.
  2. Please consider using this tool to wrap your epytext docstrings, or if you're going to continue doing it manually, at least trying to approximate its style more closely. We have very inconsistent wrapping within the Twisted codebase, a lot of it at a fill column somewhere around the 75 range rather than the documented 79, which means that whenever we edit a docstring we get a big diff of just stylistic churn.

Thanks for your work on this!

I've been seeing your name on a lot of tickets lately, by the way. Would you be interested in getting commit access?

Changed 4 years ago by Adi Roiban

Attachment: 6927-2.diff added

comment:9 Changed 4 years ago by Adi Roiban

Keywords: review added
Owner: Adi Roiban deleted

Things that need to be addressed before merging:

  1. Thanks for adding a docstring for runRequest, but unfortunately

[https://buildbot.twistedmatrix.com/builders/twistedchecker/builds/1806/steps /run-twistedchecker/logs/new%20twistedchecker%20errors this isn't really in the format our documentation tool accepts]. It looks like you've written some other epytext on this patch, so just follow that example and twistedchecker should be happy :-).

I have fixed the docstirng. Hope the new code is ok.

Using twistedchecker is very hard and while printing a lot of unrelated errors just makes me want to stop using it :(

  1. New @ivar declarations should all have @type fields to go with

them, explaining what type things are.

OK.

I have also added types for the other ivars from HTTPChannel.

  1. Private variables should be documented too. _receivedHeaderSize

should have an @ivar in the docstring as well.

OK. Done.

What is the reasoning for documenting private variables in the docstring? Users should not care... and for developers maybe a comment should be enough.

  1. Is it really kosher to just splat some response bytes as a static

string?

Why not? This is just a response to a bad client. Why waste processing power and network traffic with it?

We have the same behavior for other parts where a bad client is detected. Ex: bad command line, too many headers, bad content-length header.

I have refactored all these responses into a common method: respondToBadRequestAndDisconnect. I made it public since it is also used by Request.

See next response.

  1. Just like you should be able to customize your 500 pages, it should

be possible to customize your 400 pages as well. Right now if a web browser starts sending slightly too much header data, there will be absolutely no feedback whatsoever to the user. Given that there have been header-inflation attacks against certain applications to cause exactly this behavior, it seems like a really bad failure mode, especially since we're introducing it.

Maybe we are talking about different things... so feel free to ignore if I am on the wrong track.

The problem with bad requests from browsers, is that first the server needs to receive all data from browser/client and only then send a reponse. The browser will not read server response unless it has sent all its data... if connection fails while sending the data, the browser will try to send the data again. This is part of standard HTTP.

One option is to implement a '/dev/null' redirector for bad requests and after a bad request was detected to continue to read and discard recieved data and in the end to send our response.

I find this approach a waste of CPU and network.

If we have a browser sending 100M in the headers (ex as cookier) then we need to accept the whole 100M before sending our response, event thought after the first 16k we know that this is a bad request.

If you want I cant implement this... but this is not the approach of already existent code.

Thoughts:

  1. We really need to have some kind of system for ... ugh ...

configuration. Values like maxHeaders and totalHeadersSize are plausibly things that someone would want to tweak for a particular deployment without necessarily writing code.

I don't feel the need for that. Subclassing Request is easy and attaching a custom Request class/factory to a site is also easy... as long as it is documented... and reading the code is not hard.

I don't think that twisted.web is Django or some other other turn-key solution.

twisted.web should come with sane/secure default values and users wanting more could write custom classes.

For example, I don't think that there are many users wanting more than 500 headers.

  1. Please consider using

this tool to wrap your epytext docstrings, or if you're going to continue doing it manually, at least trying to approximate its style more closely. We have very inconsistent wrapping within the Twisted codebase, a lot of it at a fill column somewhere around the 75 range rather than the documented 79, which means that whenever we edit a docstring we get a big diff of just stylistic churn.

Looks like it does not work with Sublime3... maybe the non emacs specific part could be split into a separate package... which should be easier to port on Sublime3 and integrate with Package Control.

I tried it, but I don't like all the extra stuff it comes with... I will try to extract and reuse it as simple Sublime command.


Thanks for the svn access

I am still new to Twisted development, and until I can not help with review process I prefer to stay away from SVN and write access into the main repo. Also, I would like to continue to act as a casual Twisted developer and help with documentation and support for new developers.

As a new developer, I find it very hard to contribute to Twisted and I would like to help in making Twisted more friendly to new developers without lowering the review process and keeping the code to the same high standards.

comment:10 in reply to:  9 Changed 4 years ago by Glyph

Replying to adiroiban:

Using twistedchecker is very hard and while printing a lot of unrelated errors just makes me want to stop using it :(

Yes, twistedchecker emits lots and lots of warnings. This is why we have a buildbot that runs it for you and shows only new warnings. that's what you should be attention to (although you're welcome to try and run it locally).

What is the reasoning for documenting private variables in the docstring? Users should not care... and for developers maybe a comment should be enough.

The reasoning for documenting private variables is to make life easier for maintainers. Just as you're maintaining code written by someone else here, some day someone else will maintain this code written by you.

It's also helpful to those who may want to do something with Twisted which is not fully supported, but they could at least know that it's possible by looking at the API docs and not necessarily going through all the code manually.

I have refactored all these responses into a common method: respondToBadRequestAndDisconnect. I made it public since it is also used by Request.

See next response.

Cool. This at least gives us an extension point for the future, so that the problem could be addressed.

Maybe we are talking about different things... so feel free to ignore if I am on the wrong track.

The problem with bad requests from browsers, is that first the server needs to receive all data from browser/client and only then send a reponse. The browser will not read server response unless it has sent all its data... if connection fails while sending the data, the browser will try to send the data again. This is part of standard HTTP.

I ... don't think that's true? Where does it say that in the standard? But, in any case, if it is, it's the browser's problem to fix, not ours.

One option is to implement a '/dev/null' redirector for bad requests and after a bad request was detected to continue to read and discard recieved data and in the end to send our response.

I find this approach a waste of CPU and network.

I agree; I think a customized response is not dependent upon this.

If you want I cant implement this... but this is not the approach of already existent code.

I think what you've done is a pretty good compromise.

Thoughts:

  1. We really need to have some kind of system for ... ugh ...

configuration. Values like maxHeaders and totalHeadersSize are plausibly things that someone would want to tweak for a particular deployment without necessarily writing code.

I don't feel the need for that. Subclassing Request is easy and attaching a custom Request class/factory to a site is also easy... as long as it is documented... and reading the code is not hard.

So, let me be clear that this was in the “thoughts” section because you don't need to address it to land this branch :-).

I take issue with this assertion though. Subclassing in general is not “easy” and definitely subclassing Request is not easy at all. How do you do this within a server run by twistd web, for example? If you can't do that in an easy, documented way, then you're not talking about subclassing Request, you're talking about writing a twistd plugin and a whole load of deployment stuff and then subclassing request.

We should strive to do better for our users.

I don't think that twisted.web is Django or some other other turn-key solution.

Twisted Web's intention is definitely to be turn-key. Not in the same way as Django, but more like nginx. However, it's supposed to be turn-key as well as flexible; one doesn't mean having to abandon the other :-).

twisted.web should come with sane/secure default values and users wanting more could write custom classes.

This is contrary to Twisted's design.

An overabundance of useless configuration knobs you have to be aware of before anything works is also contrary to Twisted's design, but there should be some way for an administrator to quickly tweak these values in response to, for example, some kind of production failure caused by tripping these limits.

For example, I don't think that there are many users wanting more than 500 headers.

Fair enough.

  1. Please consider using

this tool to wrap your epytext docstrings, or if you're going to continue doing it manually, at least trying to approximate its style more closely. We have very inconsistent wrapping within the Twisted codebase, a lot of it at a fill column somewhere around the 75 range rather than the documented 79, which means that whenever we edit a docstring we get a big diff of just stylistic churn.

Looks like it does not work with Sublime3... maybe the non emacs specific part could be split into a separate package... which should be easier to port on Sublime3 and integrate with Package Control.

That file itself can be run as a script on some epytext entirely outside of Sublime. It's in that repository for pretty dumb historical reasons, and I'm sorry about that.

I tried it, but I don't like all the extra stuff it comes with... I will try to extract and reuse it as simple Sublime command.

Please do. Perhaps this should go into twisted-dev-tools?

Thanks for the svn access

Have we given it to you yet? :)

I am still new to Twisted development, and until I can not help with review process I prefer to stay away from SVN and write access into the main repo. Also, I would like to continue to act as a casual Twisted developer and help with documentation and support for new developers.

As a new developer, I find it very hard to contribute to Twisted and I would like to help in making Twisted more friendly to new developers without lowering the review process and keeping the code to the same high standards.

Great. This is a topic of ongoing discussion among Twisted developers, so I'm very happy to hear you're interested in helping. We should discuss more in a place off this ticket though.

comment:11 Changed 4 years ago by Adi Roiban

Can I trigger the buildbot for my patch before requesting a review? Is there a buildbot-try scheduler?


Understood with documenting private variables. Will try to remember to document them in the future.


For the browser behavior of connection closed/responsed sent before client sends full request you can check nginx documentation or this discussion

http://stackoverflow.com/questions/4947107/nginx-upload-client-max-body-size-issue


While working with twisted I did not had the feeling that it is a turn-key solution, rather a flexible framework. I don't have complex use case, but I always had to implement my own subclasses. If you want, we can continue this discussion elsewhere, but I am happy with current Twisted configuration so I am not able to suggest improvements.


I don't have SVN access and I don't need it now. Thanks for the offer :)


So, what needs to be changed to the current patch in order to have it merged?

Thanks!

comment:12 Changed 4 years ago by lvh

Two points I can definitely answer:

1) pip install twisted-dev-tools will give you a force-build script; see examples here: https://github.com/twisted/twisted-dev-tools 2) the private variable documentation part is definitely mandatory; but I'll leave it to glyph to give you an exhaustive list.

comment:13 in reply to:  12 Changed 4 years ago by Adi Roiban

Replying to lvh:

Two points I can definitely answer:

1) pip install twisted-dev-tools will give you a force-build script; see examples here: https://github.com/twisted/twisted-dev-tools

Reading the docs, it looks like this does not work for normal contributors, since it requires SVN access and is not using a try scheduler.

I have experience with configuring try scheduler or Travis-CI and maybe we can create a minimum builder for people without SVN access... will follow on the main mailinglist.

2) the private variable documentation part is definitely mandatory; but I'll leave it to glyph to give you an exhaustive list.

Thanks. I already got the exhaustive answer :)

comment:14 Changed 4 years ago by lvh

Yes and no; you can force builds if someone else has committed it to SVN first :) So, technically no, but for practical purposes usually yes.

comment:15 Changed 4 years ago by Jean-Paul Calderone

I have experience with configuring try scheduler or Travis-CI and maybe we can create a minimum builder for people without SVN access... will follow on the main mailinglist.

This is intentionally not configured. We don't have the infrastructure to run arbitrary code from anyone on the internet.

comment:16 in reply to:  15 Changed 4 years ago by Glyph

Replying to exarkun:

I have experience with configuring try scheduler or Travis-CI and maybe we can create a minimum builder for people without SVN access... will follow on the main mailinglist.

This is intentionally not configured. We don't have the infrastructure to run arbitrary code from anyone on the internet.

If Adi could set up our non-platform-specific builders on Travis-CI, presumably they can take care of the sandboxing for us, though, and we wouldn't have to set anything up because he's volunteered to do the work :).

comment:17 Changed 4 years ago by Glyph

Branch: branches/header-limits-6927branches/header-limits-6927-2

(In [42783]) Branching to header-limits-6927-2.

comment:18 Changed 4 years ago by Glyph

Keywords: review removed
Owner: set to Adi Roiban

Thanks adi!

  1. respondToBadRequestAndDisconnect ought to be a private method; overriding this would be a horrible way to customize this behavior (and it's not really behavior that I can see a use-case for customizing).
  2. runRequest shouldn't change the default for its success parameter, since that requires changing a bunch of other callers (more callers than pass False for success, at least judging by the diff).
  3. If you add a new parameter to a method like you added channel to runRequest, in the future, always add to the end of the signature. For example, in this case, you have silently changed the meaning of test_basicAuth by accident, because it positionally passes success (now channel as 0), and it now only passes by accident.
  4. The documentation on MAX_LENGTH ought to be @ivar MAX_LENGTH, since it is in fact treated as an instance variable. Except... your assessment that MAX_LENGTH limits the length of individual headers is incorrect. If you'll notice the elif line[0] in b' \t': branch of lineReceived, you can see that we have no limit on the size of an individual header, because it can have an unlimited number of continuation lines. If you're going to fix this please fix it in a separate patch so we don't start growing this; for now I think the best thing to do would be to just eliminate this doc change since MAX_LENGTH doesn't actually enforce anything.

Changed 4 years ago by Adi Roiban

Attachment: 6972-3.diff added

comment:19 Changed 4 years ago by Adi Roiban

Keywords: review added
Owner: Adi Roiban deleted

respondToBadRequestAndDisconnect ought to be a private method; overriding this would be a horrible way to customize this behavior (and it's not really behavior that I can see a use-case for customizing).

The problem is that in order to reduce code duplication, this method is called from http.Request ... so it needs to be public.

If I will make it private, it would be ugly to call it from http.Request .. but it would also be ugly to duplicate the code in http.Request

Please advise how to proceed!

runRequest shouldn't change the default for its success parameter, since that requires changing a bunch of other callers (more callers than pass False for success, at least judging by the diff).

I have reverted the default value. For me, false, was a better default value... and the diff was small.

I went for default False , since 'True' depends on an external didReponse member. In the same time, I think that instead of having a flag argument, a new method should have been created

    self.runRequest(content, factory)
    self.runRequestWithSuccess(content, factory)

If you add a new parameter to a method like you added channel to runRequest, in the future, always add to the end of the signature. For example, in this case, you have silently changed the meaning of test_basicAuth by accident, because it positionally passes success (now channel as 0), and it now only passes by accident.

True. My bad. I was relying to much on the fact that test passed and did not checked for other side effects. I have moved the channel to the end.

The documentation on MAX_LENGTH ought to be @ivar MAX_LENGTH, since it is in fact treated as an instance variable. Except... your assessment that MAX_LENGTH limits the length of individual headers is incorrect. If you'll notice the elif line[0] in b' \t': branch of lineReceived, you can see that we have no limit on the size of an individual header, because it can have an unlimited number of continuation lines. If you're going to fix this please fix it in a separate patch so we don't start growing this; for now I think the best thing to do would be to just eliminate this doc change since MAX_LENGTH doesn't actually enforce anything.

I think that MAX_LENGTH does enforce the initial request size (in case someone has a huge url or GET parameters) It also enforce a single line in the header :)

Moved documentation as ivar.

I am new to HTTP and I was not aware that HTTP allows multi line headers. I tried to add some comments to the lineRecied method so that future readers will notice this. Hope it help. Let me know if you find them useless and I will remove them.


I have attached a patch based on the new branch

Many thanks for the review!

comment:20 in reply to:  19 Changed 3 years ago by Glyph

Replying to adiroiban:

respondToBadRequestAndDisconnect ought to be a private method; overriding this would be a horrible way to customize this behavior (and it's not really behavior that I can see a use-case for customizing).

The problem is that in order to reduce code duplication, this method is called from http.Request ... so it needs to be public.

If I will make it private, it would be ugly to call it from http.Request .. but it would also be ugly to duplicate the code in http.Request

So what I actually object to is not that it's “public” in that sense, but rather that it’s “published”, i.e. that it is exposed as a part of the interface that twisted exposes to the rest of the world.

Please advise how to proceed!

Ideally this would be on something that is named “publicly” in such a way that both http.Request and http.HTTPChannel could access it, but “privately” as far as the rest of the world is concerned. One way to do that might be to put it in a privately-named module.

runRequest shouldn't change the default for its success parameter, since that requires changing a bunch of other callers (more callers than pass False for success, at least judging by the diff).

I have reverted the default value. For me, false, was a better default value... and the diff was small.

Thanks. Perhaps we can do a separate ticket later for test refactoring.

I went for default False , since 'True' depends on an external didReponse member. In the same time, I think that instead of having a flag argument, a new method should have been created

    self.runRequest(content, factory)
    self.runRequestWithSuccess(content, factory)

This is also a perfectly reasonable approach. I just want to keep the changes easy to review and as task-specific as possible ;-).

If you add a new parameter to a method like you added channel to runRequest, in the future, always add to the end of the signature. For example, in this case, you have silently changed the meaning of test_basicAuth by accident, because it positionally passes success (now channel as 0), and it now only passes by accident.

True. My bad. I was relying to much on the fact that test passed and did not checked for other side effects. I have moved the channel to the end.

The specific case is bad, but more importantly it makes a nice example of why you should be careful about maintaining compatibility with existing behavior, especially in test helpers :-).

The documentation on MAX_LENGTH ought to be @ivar MAX_LENGTH, since it is in fact treated as an instance variable. Except... your assessment that MAX_LENGTH limits the length of individual headers is incorrect. If you'll notice the elif line[0] in b' \t': branch of lineReceived, you can see that we have no limit on the size of an individual header, because it can have an unlimited number of continuation lines. If you're going to fix this please fix it in a separate patch so we don't start growing this; for now I think the best thing to do would be to just eliminate this doc change since MAX_LENGTH doesn't actually enforce anything.

I think that MAX_LENGTH does enforce the initial request size (in case someone has a huge url or GET parameters)

It limits the size of some parts of the initial request, but it doesn't limit the size of the entire request.

It also enforce a single line in the header :)

I'm not sure what you mean?

Moved documentation as ivar.

Thanks.

I am new to HTTP and I was not aware that HTTP allows multi line headers.

The RFC might be a fun read, if you've never gone through it :).

I tried to add some comments to the lineRecied method so that future readers will notice this.

Thanks. (And when you add comments like this, RFC citations are always helpful.)

Hope it help. Let me know if you find them useless and I will remove them.

Hopefully someone (maybe me) will do a full review again soon.

I have attached a patch based on the new branch

Thanks, I've applied it and kicked the buildbots.

Many thanks for the review!

Don't thank me, just do a review for someone else :). Might I suggest this one?.

comment:21 Changed 3 years ago by Glyph

Keywords: review removed
Owner: set to Adi Roiban

Hi, Adi. Again, sorry for the slow pace of reviews. Thanks for your continued contributions.

Buildbot looks good.

  1. I still don't like respondToBadRequestAndDisconnect though. Its docstring even says it's a quick and dirty hack, which is exactly the sort of thing we should avoid exposing in public API. Here's my proposed solution: since (A) everything in here is in http.py, (B) transport and channel are already public attributes, and (C) there should be no real need for extensibility of this interface, since it's a serialization concern ("bad request" means "you didn't serialize the request correctly") and this code is the layer that implements deserialization, I think it should just be a private function _respondToBadRequestAndDisconnect, that takes an HTTPChannel. It technically only requires a transport, but this way if we want to extend its functionality to manipulate the channel in some other way beyond its transport, we can easily do that.
  2. You've got a @type maxHeaders where I think you meant @type _transferDecoder.
  3. The comment next to maxHeaders is somewhat redundant with its @ivar; please remove the comment.
  4. The indentation you've chosen in a few places, while it doesn't violate any automatically-enforced coding standards, is peculiar:
    1. runRequest's indentation is inconsistent; "channel" and the closing parenthesis are indented further than "self". My personal preference is to indent long signatures like this:
          def runRequest(self, httpRequest, requestFactory=None, success=True,
                         channel=None):
      
      so that there's a dedent before the implementation.
    2. requestFactory's type documentation is inaccurate; it's not a Request, it's a 2-argument callable returning a Request.
    3. The comments are also indented peculiarly. Rather than
      # End of headers.
      elif line == b'':
      
      I would have gone with
      elif line == b'':
          # End of headers.
      
      That makes it clearer (to me, anyway) that the comment is about that particular indented else suite - as it currently is, I read it like it's addressing all of the subsequent suites, or the end of the previous suite, or something.

If it weren't for the first point, I would probably address this all myself and then land it, but I think that is mandatory feedback. As long as I was bouncing it back to you for that I figured I'd include the stylistic stuff so you could fix that too, and for next time. Please feel free to badger me mercilessly for a re-review so we can land this promptly :-).

comment:22 Changed 3 years ago by Jean-Paul Calderone

My personal preference is to indent long signatures like this

This isn't just preference, this is in the coding standard (inherited from PEP 8).

comment:23 in reply to:  22 Changed 3 years ago by Glyph

Replying to exarkun:

My personal preference is to indent long signatures like this

This isn't just preference, this is in the coding standard (inherited from PEP 8).

My personal preference is considerably more prescriptive than PEP 8.

The relevant section of PEP 8 is the indentation section, where it gives an example of a hanging indent for a parameter list as well as my preferred style (no hanging indent). I don't like using hanging indents for parameter lists, because tool support for the requisite additional indent is poor; automatic indentation in vim, emacs, and sublime all ignore it and indent as if it were regular parentheses.

comment:24 Changed 3 years ago by Adi Roiban

Keywords: review added
Owner: Adi Roiban deleted

Thanks.

  1. OK. I went for private function. Thanks for the suggestion!
  1. True. Fixed.
  1. I don't know about what comment are you talking about... hope that soon we can do review in github

4.1 Yep My bad. I wanted to do it your way but I forgot to add the extra space.

My preferred indentation is

# More indentation included to distinguish this from the rest.

I guess that your preferred way is

# Aligned with opening delimiter.

I don't like this method as then I will have to add up to 3 extra spaces to align it.

4.2 Done.

4.3 Done.


I have attached a new diff based on latest trunk.

Feel free to modify and merge it.

Thanks!

Changed 3 years ago by Adi Roiban

Attachment: 6927-4.diff added

comment:25 Changed 3 years ago by Glyph

Branch: branches/header-limits-6927-2branches/header-limits-6927-3

(In [43453]) Branching to header-limits-6927-3.

comment:26 Changed 3 years ago by Glyph

Branch: branches/header-limits-6927-3branches/header-limits-6927-4

(In [43947]) Branching to header-limits-6927-4.

comment:27 Changed 3 years ago by Glyph

Don't know what happened there with that last branch, I guess I got distracted right after making it. New branch made with the new patch, builds run.

comment:28 Changed 3 years ago by Glyph

Keywords: review removed
Owner: set to Adi Roiban

Hi adi,

Sorry for the latency on this one. Luckily the queue is much shorter these days - thanks for that :).

Looks like this has a failing test, more or less due to a typo, and a missing epydoc field. I don't see a whole lot more to give feedback on though, I suspect that the next review will pass if the buildbots are green...

comment:29 Changed 3 years ago by Adi Roiban

Keywords: review added
Owner: Adi Roiban deleted

Thanks for the review.

I have fixed the errors and committed the changes to be checked by buidlbot.

Please take another look at latest changes.

Thanks!

comment:30 Changed 3 years ago by Glyph

Keywords: review removed
Owner: set to Adi Roiban

I think we're good to go now. Please land.

Thanks!

comment:31 Changed 3 years ago by Adi Roiban

Resolution: fixed
Status: newclosed

(In [44067]) Merging header-limits-6927-4: Reduce the limit the HTTP headers size.

Author: adiroiban Reviewer: glyph Fixes: #6927

Note: See TracTickets for help on using tickets.