-
-
Notifications
You must be signed in to change notification settings - Fork 1.5k
Make HTTP_MAX_HEADER_SIZE configurable via gyp #452
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM % style nit.
http_parser.gyp
Outdated
'include_dirs': [ '.' ], | ||
}, | ||
'defines': [ 'HTTP_PARSER_STRICT=0' ], | ||
'defines': [ 'HTTP_MAX_HEADER_SIZE=<(http_max_header_size)', 'HTTP_PARSER_STRICT=0' ], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you put the directives on their own lines here and below?
}, | ||
|
||
'variables': { | ||
'http_max_header_size%': '8192' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@bnoordhuis do you think we should change this back to 80KB here? We will be passing down the variable anyway in Node.js.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess it won't hurt backwards compatibility but truthfully, I picked the 80 kb limit at random.
}, | ||
|
||
'variables': { | ||
'http_max_header_size%': '8192' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess it won't hurt backwards compatibility but truthfully, I picked the 80 kb limit at random.
Spin out from nodejs/node#24716.
This also sets the default to 8KB when compiled via gyp.