Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

deps: update http_parser to 2.4.0 #345

Closed

Conversation

indutny
Copy link
Member

@indutny indutny commented Jan 13, 2015

@indutny indutny added this to the v1.0.0 milestone Jan 13, 2015
@yosuke-furukawa
Copy link
Member

+1 !!! Thanks for quick release.

@rvagg
Copy link
Member

rvagg commented Jan 14, 2015

+1 just for the goto fail additions

how safe is this to land straight into 1.0.0 @indutny? I don't have a feel for how major (and potentially dangerous) these changes are.

@indutny
Copy link
Member Author

indutny commented Jan 14, 2015

@rvagg I feel safe about it, every commit was reviewed by either me or @bnoordhuis

@rvagg
Copy link
Member

rvagg commented Jan 14, 2015

ok LGTM then

indutny added a commit that referenced this pull request Jan 14, 2015
PR-URL: #345
Reviewed-By: Rod Vagg <rod@vagg.org>
@indutny
Copy link
Member Author

indutny commented Jan 14, 2015

Landed in d790f61, thank you.

@indutny indutny closed this Jan 14, 2015
@indutny indutny deleted the feature/update-http_parser-to-2.4 branch January 14, 2015 00:11
@yosuke-furukawa
Copy link
Member

I just benchmarked using benchmark/http.sh .
http is improved! Thanks.

node v0.11.14 iojs v1.x brunch(d790f61)
1 15724.83 17600.49
2 16801.4 18409.31
3 17080.68 18736.1
4 17347.16 18726.99
5 16961.31 18175.39
6 16508.48 17537.85
7 17193.63 18224.03
8 17047.39 18455.47
9 17002.99 18342.36
10 17735.59 18652.9
11 17559.13 18593
12 17607.01 18704.86
13 17159.27 18571.3
14 17524.88 18468.55
15 17302.25 18570.18
16 17604.3 16193.09
17 17684.03 15473.57
18 17282.97 17837.95
19 17567.96 17696.15
20 17471.96 17748.02
Average 17208.361 18035.878

machine spec:
CPU : 2.3 GHz Intel Core i7
Memory : 16GB
OSX Yosemite

@indutny
Copy link
Member Author

indutny commented Jan 14, 2015

Sorry, this is going to be postponed until next release. It breaks build on windows and some tests became flacky (according to users).

@yosuke-furukawa
Copy link
Member

hm, no problem. i am looking forward to next release.

@jbergstroem
Copy link
Member

Test fail with http_parser 2.4.0 on OS X:

=== release test-http-1.0 ===                                                  
Path: parallel/test-http-1.0
assert.js:100
  throw new assert.AssertionError({
        ^
AssertionError: 1 == 2
    at response_validator (/private/tmp/io.js/test/parallel/test-http-1.0.js:154:12)
    at Socket.<anonymous> (/private/tmp/io.js/test/parallel/test-http-1.0.js:48:7)
    at Socket.emit (events.js:117:20)
    at _stream_readable.js:888:16
    at process._tickCallback (node.js:337:11)
Command: out/Release/iojs /private/tmp/io.js/test/parallel/test-http-1.0.js

@indutny
Copy link
Member Author

indutny commented Jan 14, 2015

@jbergstroem is it always failing? Appears to be working on my side.

@jbergstroem
Copy link
Member

@indutny Just tried once (as well as after pulling the revert). Let me check.

Edit: Nope, can't reliably reproduce.

@indutny
Copy link
Member Author

indutny commented Jan 14, 2015

@jbergstroem I think this is just a test problem. I built http-parser with ASAN, and sanitizers for various overflows, and got no errors and no test failures.

@jbergstroem
Copy link
Member

@indutny I haven't seen it since; don't take it into consideration until I can give reliable info.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants