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.

Sign up for GitHub

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

Jump to bottom

Backport to v1.x #1736

Closed
wants to merge 48 commits into from
Closed

Backport to v1.x #1736

wants to merge 48 commits into from

Conversation

Fishrock123
Copy link
Contributor

Basically everything we could backport to v1.x, including some build & perf improvements, I'm not too sure what we all want to take.

Commits have been slightly re-arranged in some cases to group related commits together.

I guess we should discuss what we do / don't want to take? I'm willing to do the grunt work of any metadata.

cc @rvagg @nodejs/lts (need to make that group soon..) maybe also cc @jasnell, he may have opinions?

Note: don't try to view the changes here, just don't.

JacksonTian and others added 30 commits May 19, 2015 12:20
@JacksonTian @Fishrock123
buffer: little improve for Buffer.concat method
fa105ab 
When buffer list less than 2, no need to calculate the length.
The change's benchmark result is here:
https://gist.github.com/JacksonTian/2c9e2bdec00018e010e6
It improve 15% ~ 25% speed when list only have one buffer,
to other cases no effect.

PR-URL: nodejs#1437
Reviewed-By: Trevor Norris <trev.norris@gmail.com>
Reviewed-By: Brendan Ashworth <brendan.ashworth@me.com>
@bnoordhuis @Fishrock123
src: fix NODE_DEPRECATED macro
1ab65e4 
The NODE_DEPRECATED macro was piggybacking on the V8_DEPRECATED macro
but that macro is silent unless V8_DEPRECATION_WARNINGS is defined,
something io.js doesn't do.  Ergo, no deprecation notices were being
issued.

PR-URL: nodejs#1565
Reviewed-By: Trevor Norris <trev.norris@gmail.com>
@bnoordhuis @Fishrock123
src: fix deprecation warnings
3c9f63f 
The previous commit enables deprecation warnings, this commit fixes
the handful of offending sites where the isolate was not explicitly
being passed around.

PR-URL: nodejs#1565
Reviewed-By: Trevor Norris <trev.norris@gmail.com>
@r-52 @Fishrock123
gitignore: ignore xcode workspaces and projects
a76b6ba 
Calling ./configure --xcode creates xcode projects and a
workspace for io.js.

PR-URL: nodejs#1562
Reviewed-By: Vladimir Kurchatkin <vladimir.kurchatkin@gmail.com>
Reviewed-By: Chris Dickinson <christopher.s.dickinson@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Roman Reiss <me@silverwind.io>
@jbergstroem @Fishrock123
build: Use option groups in configure output
91da332 
Minor edits to current build flags and its help texts as well
as grouping shared and i18n options into separate option groups.

Also, validate i18n default/logic similar to how we treat other
options. `--download` isn't really intl-specific but is only used
for that purpose which is why it's grouped similarly.

PR-URL: nodejs#1533
Reviewed-By: Rod Vagg <rod@vagg.org>
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
Reviewed-By: Steven R. Loomis <srl@icu-project.org>
@jbergstroem @Fishrock123
build: move --with-intl to intl optgroup
f6d7de1 
PR-URL: nodejs#1680
Reviewed-By: Rod Vagg <rod@vagg.org>
@indutny @Fishrock123
stream_base: dispatch reqs in the stream impl
c5d7c7e 
Dispatch requests in the implementation of the stream, not in the code
creating these requests. The requests might be piled up and invoked
internally in the implementation, so it should know better when it is
the time to dispatch them.

In fact, TLS was doing exactly this thing which led us to...

Fix: nodejs#1512
PR-URL: nodejs#1563
Reviewed-By: Shigeki Ohtsu <ohtsu@iij.ad.jp>
@indutny @Fishrock123
tls: use SSL_set_cert_cb for async SNI/OCSP
592230a 
Do not enable ClientHello parser for async SNI/OCSP. Use new
OpenSSL-1.0.2's API `SSL_set_cert_cb` to pause the handshake process and
load the cert/OCSP response asynchronously. Hopefuly this will make
whole async SNI/OCSP process much faster and will eventually let us
remove the ClientHello parser itself (which is currently used only for
async session, see nodejs#1462 for the discussion of removing it).

NOTE: Ported our code to `SSL_CTX_add1_chain_cert` to use
`SSL_CTX_get0_chain_certs` in `CertCbDone`. Test provided for this
feature.

Fix: nodejs#1423
PR-URL: nodejs#1464
Reviewed-By: Shigeki Ohtsu <ohtsu@iij.ad.jp>
@mscdex @Fishrock123
node: improve nextTick performance
908643d 
This commit uses separate functions to isolate deopts caused by
try-catches and avoids fn.apply() for callbacks with small numbers
of arguments.

These changes improve performance by ~1-40% in the various
nextTick benchmarks.

PR-URL: nodejs#1571
Reviewed-By: Trevor Norris <trev.norris@gmail.com>
Reviewed-By: Chris Dickinson <christopher.s.dickinson@gmail.com>
@indutny @Fishrock123
net: ensure Write/ShutdownWrap references handle
26760a9 
`StreamBase::AfterWrite` is passing handle as an argument to the
`afterWrite` function in net.js. Thus GC should not collect the handle
and the request separately and assume that they are tied together.

With this commit - request will always outlive the StreamBase instance,
helping us survive the GC pass.

Same applies to the ShutdownWrap instances, they should never be
collected after the StreamBase instance.

Fix: nodejs#1580
PR-URL: nodejs#1590
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
@ChALkeR @Fishrock123
tls_wrap: Unlink TLSWrap and SecureContext objects
dedddb7 
This makes `TLSWrap` and `SecureContext` objects collectable by the
incremental gc.

`res = null` destroys the cyclic reference in the `reading` property.
`this.ssl = null` removes the remaining reference to the `TLSWrap`.
`this.ssl._secureContext.context = null` removes the reference to
the `SecureContext` object, even though there might be references
to `this.ssl._secureContext` somewhere.

The `reading` property will now throw an error if accessed after the
socket is closed, but that should not happen.

PR-URL: nodejs#1580
Reviewed-By: Fedor Indutny <fedor@indutny.com>
Reviewed-By: Shigeki Ohtsu <ohtsu@iij.ad.jp>
@bnoordhuis @Fishrock123
src: fix -Wmissing-field-initializers warning
6568dc9 
Fix the following (non-serious) compiler warning:

    ../src/node_http_parser.cc:558:1: warning: missing initializer for
    member 'http_parser_settings::on_chunk_header'
    [-Wmissing-field-initializers]
    };
    ^
    ../src/node_http_parser.cc:558:1: warning: missing initializer for
    member 'http_parser_settings::on_chunk_complete'
    [-Wmissing-field-initializers]

Introduced in commit b3a7da1 ("deps: update http_parser to 2.5.0").

PR-URL: nodejs#1606
Reviewed-By: Fedor Indutny <fedor@indutny.com>
@trevnorris @Fishrock123
async-wrap: don't call init callback unnecessarily
a2b6650 
Some calls to ReqWrap would get through the initial check and allow the
init callback to run, even though the callback had not been used on the
parent. Fix by explicitly checking if the parent has a queue.

Also change the name of the check, and internal field of AsyncHooks.
Other names were confusing.

PR-URL: nodejs#1614
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
@trevnorris @Fishrock123
async-wrap: pass PROVIDER as first arg to init
c8b05e4 
Allow the init callback to see the PROVIDER type easily by being able to
compare the flag with the list of providers on the exported async_wrap
object.

PR-URL: nodejs#1614
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
@trevnorris @Fishrock123
async-wrap: set flags using functions
a703e07 
Setting flags using cryptic numeric object fields is confusing. Instead
use much simpler .enable()/.disable() calls on the async_wrap object.

PR-URL: nodejs#1614
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
@trevnorris @Fishrock123
async-wrap: remove before/after calls in init
54ae094 
It doesn't make sense to call before/after callbacks in init to the
parent because they'll be made anyway from MakeCallback. If information
does need to be propagated then it should be done automatically. Will
deal with this if the issue arrises in the future.

PR-URL: nodejs#1614
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
@bnoordhuis @Fishrock123
src: fix NODE_DEPRECATED macro with old compilers
d90eadc 
The `__attribute__((deprecated("warning")))` macro didn't exist until
gcc 4.5 and clang 2.9.

While io.js does not build with compilers that old, add-ons do.  Let's
make src/node.h compatible with such compilers, it's a public header.

PR-URL: nodejs#1626
Refs: nodejs#1619
Reviewed-By: Rod Vagg <rod@vagg.org>
Reviewed-By: Trevor Norris <trev.norris@gmail.com>
@bnoordhuis @Fishrock123
src: fix pedantic cpplint whitespace warnings
df389c6 
Introduced in commit b712af7 ("src: fix NODE_DEPRECATED macro with old
compilers").

PR-URL: nodejs#1640
Reviewed-By: Roman Reiss <me@silverwind.io>
@Fishrock123
js_stream: fix buffer index in DoWrite
46561fc 
The index of buffer to write in JSStream was always 0 by mistake. This
fix was to use increment index of buffer arrays.
The test was originally made by Brian White in nodejs#1594.

Fix: nodejs#1595
Fix: nodejs#1594
PR-URL: nodejs#1635
Reviewed-By: Fedor Indutny <fedor@indutny.com>
@saghul @Fishrock123
deps: update libuv to 1.5.0
8651ecc 
Fixes: nodejs#1397
Fixes: nodejs#1512
Fixes: nodejs#1621
Fixes: nodejs#862
PR-URL: nodejs#1646
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
@ivan @Fishrock123
src: export the ParseEncoding function on Windows
98b1109 
Makes the ParseEncoding symbol visible to addons on Windows.
It was already visible on Unices.

PR-URL: nodejs#1596
Reviewed-By: Fedor Indutny <fedor@indutny.com>
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
@thefourtheye @Fishrock123
install: fix NameError
e628b19 
If `len(args)` is less than two, then
`install_path = dst_dir + node_prefix + '/'` would throw a `NameError`,
because `dst_dir` will not be defined yet. So we are assigning `''` as
the default value.

PR-URL: nodejs#1628
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
@r-52 @Fishrock123
src: add type check to v8.setFlagsFromString()
e42dcc2 
Calling v8.setFlagsFromString with e.g a function as a flag argument
gave no exception or warning that the function call will fail.

There is now an exception if the function gets called with the wrong
flag type (string is required) or that a flag is expected.

Other APIs already provide exceptions if the argument has not the
expected type.

PR-URL: nodejs#1652
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
@Olegas @Fishrock123
cluster: disconnect event not emitted correctly
ebc671b 
Inside of a worker, disconnect event was not emitted on cluster.worker

Fixes: nodejs#1304
PR-URL: nodejs#1386
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
@yosuke-furukawa @Fishrock123
tools: replace closure-linter with eslint
367e221 
PR-URL: nodejs#1539
Fixes: nodejs#1253
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
Reviewed-By: Trevor Norris <trev.norris@gmail.com>
Reviewed-By: Roman Reiss <me@silverwind.io>
Reviewed-By: Chris Dickinson <christopher.s.dickinson@gmail.com>
Reviewed-By: Johan Bergström <bugs@bergstroem.nu>
Reviewed-By: Fedor Indutny <fedor.indutny@gmail.com>
@yosuke-furukawa @Fishrock123
lib: fix eslint styles
edb4cb6 
PR-URL: nodejs#1539
Fixes: nodejs#1253
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
Reviewed-By: Trevor Norris <trev.norris@gmail.com>
Reviewed-By: Roman Reiss <me@silverwind.io>
Reviewed-By: Chris Dickinson <christopher.s.dickinson@gmail.com>
Reviewed-By: Johan Bergström <bugs@bergstroem.nu>
Reviewed-By: Fedor Indutny <fedor.indutny@gmail.com>
@Fishrock123
tools: remove closure_linter to eslint on windows
82a5dab 
PR-URL: nodejs#1685
Reviewed-By: Johan Bergström <bugs@bergstroem.nu>
Reviewed-By: Roman Reiss <me@silverwind.io>
@silverwind @Fishrock123
tools: set eslint comma-spacing to 'warn'
ecb2436 
Certain cases with comments inside arrays or object literals fail to
pass eslint's comma-spacing rule. This change sets the comma-spacing
rule to the 'warn' level.

Once eslint/eslint#2408 is resolved and
released, this rule should be set back to 'error' level.

PR-URL: nodejs#1672
Reviewed-By: Yosuke Furukawa <yosuke.furukawa@gmail.com>
@silverwind @Fishrock123
tools: make eslint work on subdirectories
91a9c6d 
The old pattern didn't include files in lib/internal. This changes the
pattern to directories which makes eslint apply to all subdirectories as
well.

PR-URL: nodejs#1686
Reviewed-By: Chris Dickinson <christopher.s.dickinson@gmail.com>
@rlidwka @Fishrock123
readline: turn emitKeys into a streaming parser
c212f8e 
In certain environments escape sequences could be splitted into
multiple chunks. For example, when user presses left arrow,
`\x1b[D` sequence could appear as two keypresses (`\x1b` + `[D`).

PR-URL: nodejs#1601
Fixes: nodejs#1403
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
Reviewed-By: Roman Reiss <me@silverwind.io>
yosuke-furukawa and others added 4 commits May 19, 2015 13:42
@yosuke-furukawa @Fishrock123
src: fix preload when used with prior flags
df9aac8 
Fixes: nodejs#1691
PR-URL: nodejs#1694
Reviewed-By: Ali Ijaz Sheikh <ofrobots@google.com>
Reviewed-By: Trevor Norris <trev.norris@gmail.com>
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
@Fishrock123
tls: fix tls handshake check in ssl error
0e9be82 
In ssl.onerror event, `this` refers `ssl` so that
`this._secureEstablished` is always undefined. Fix it to refer
TLSSocket.

PR-URL: nodejs#1661
Reviewed-By: Fedor Indutny <fedor@indutny.com>
@Fishrock123
tls_wrap: fix error cb when fatal TLS Alert recvd
4ba3d68 
SSL_read() returns 0 when fatal TLS Alert is received.
Fix to invoke ssl error callback in this case.

PR-URL: nodejs#1661
Reviewed-By: Fedor Indutny <fedor@indutny.com>
@trevnorris @Fishrock123
core: set PROVIDER type as Persistent class id
575738b 
Pass along the PROVIDER type, that is already passed to AsyncWrap, along
to BaseObject to set the handle_'s class id. This will allow all
Persistents to be transversed and uniquely identified by what type they
are using APIs such as v8::PersistentHandleVisitor.

PR-URL: nodejs#1730
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
@Fishrock123
Copy link
Contributor Author

Slight addendum: 91da332 and f6d7de1 should be grouped together if we are taking them.

@Fishrock123
Copy link
Contributor Author

CI: https://jenkins-iojs.nodesource.com/job/iojs+any-pr+multi/696/

@jasnell
Copy link
Member

jasnell commented May 19, 2015

Will look in detail tomorrow.
On May 19, 2015 10:48 AM, "Jeremiah Senkpiel" notifications@github.com
wrote:

Basically everything we could backport to v1.x, including some build &
perf improvements, I'm not too sure what we all want to take.

Commits have been slightly re-arranged in some cases to group related
commits together.

I guess we should discuss what we do / don't want to take?

cc @rvagg https://github.com/rvagg @nodejs/lts (need to make that group
soon..) maybe also cc @jasnell https://github.com/jasnell, he may have
opinions?

Note; don't try to view the changes here, just don't.

You can view, comment on, or merge this pull request online at:

#1736
Commit Summary

  • buffer: little improve for Buffer.concat method
  • src: fix NODE_DEPRECATED macro
  • src: fix deprecation warnings
  • gitignore: ignore xcode workspaces and projects
  • build: Use option groups in configure output
  • tls: use SSL_set_cert_cb for async SNI/OCSP
  • stream_base: dispatch reqs in the stream impl
  • node: improve nextTick performance
  • build: move --with-intl to intl optgroup
  • tls_wrap: Unlink TLSWrap and SecureContext objects
  • net: ensure Write/ShutdownWrap references handle
  • src: fix -Wmissing-field-initializers warning
  • async-wrap: don't call init callback unnecessarily
  • async-wrap: pass PROVIDER as first arg to init
  • async-wrap: set flags using functions
  • async-wrap: remove before/after calls in init
  • src: fix NODE_DEPRECATED macro with old compilers
  • src: fix pedantic cpplint whitespace warnings
  • src: export the ParseEncoding function on Windows
  • deps: update libuv to 1.5.0
  • js_stream: fix buffer index in DoWrite
  • cluster: disconnect event not emitted correctly
  • install: fix NameError
  • src: add type check to v8.setFlagsFromString()
  • tools: replace closure-linter with eslint
  • lib: fix eslint styles
  • tools: set eslint comma-spacing to 'warn'
  • tools: remove closure_linter to eslint on windows
  • tools: make eslint work on subdirectories
  • dgram: call send callback asynchronously
  • readline: turn emitKeys into a streaming parser
  • test: fix infinite loop detection
  • build: re-enable V8 snapshots
  • tls: update default ciphers to use gcm and aes128
  • deps: sync with upstream c-ares/c-ares@bba4dc5
  • deps: provide TXT chunk info in c-ares
  • src,deps: replace LoadLibrary by LoadLibraryW
  • events: provide better error message for unhandled error
  • tools: refactor make test-npm into test-npm.sh
  • src: fix preload when used with prior flags
  • build: use backslashes for paths on windows
  • tls: fix tls handshake check in ssl error
  • tls_wrap: fix error cb when fatal TLS Alert recvd
  • core: set PROVIDER type as Persistent class id

File Changes

Patch Links:

—
Reply to this email directly or view it on GitHub
#1736.

@Fishrock123
Copy link
Contributor Author

Not sure what this is, smartos14-64

not ok 131 - test-debug-signal-cluster.js
# got pids [30889,30890,30891]
# 
# assert.js:88
# throw new assert.AssertionError({
# ^
# AssertionError: test timed out.
# at testTimedOut [as _onTimeout] (/home/iojs/build/workspace/iojs+any-pr+multi/nodes/smartos14-64/test/parallel/test-debug-signal-cluster.js:53:3)
# at Timer.unrefdHandle (timers.js:304:14)
# > all workers are running
# > Starting debugger agent.
# > Debugger listening on port 12388
# > Starting debugger agent.
# > Starting debugger agent.
# > Debugger listening on port 12390Debugger listening on port 12389

@Fishrock123
Copy link
Contributor Author

I also backported the linter changes because it's a candidate to cause conflicts in the future if it isn't.

@Fishrock123
Copy link
Contributor Author

@jeisinger, was the security issue for v8 snapshots already fixed in v8 4.1 (what 1.8.x uses)?

@jeisinger
Copy link
Contributor

Yes, it's fixed already for several releases before that

On Wed, May 20, 2015, 6:00 PM Jeremiah Senkpiel notifications@github.com
wrote:

@jeisinger https://github.com/jeisinger, was the security issue for v8
snapshots already fixed in v8 4.1 (what 1.8.x uses)?

—
Reply to this email directly or view it on GitHub
#1736 (comment).

@jasnell
Copy link
Member

jasnell commented May 21, 2015

@Fishrock123 nothing in this batch sticks out as being potentially problematic.

silverwind and others added 4 commits May 21, 2015 13:26
@silverwind @Fishrock123
test: enable linting for tests
9f8a3a4 
Enable linting for the test directory. A number of changes was made so
all tests conform the current rules used by lib and src directories. The
only exception for tests is that unreachable (dead) code is allowed.

test-fs-non-number-arguments-throw had to be excluded from the changes
because of a weird issue on Windows CI.

PR-URL: nodejs#1721
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>

Conflicts:
	test/parallel/test-arm-math-exp-regress-1376.js
	test/parallel/test-net-dns-custom-lookup.js
	test/parallel/test-net-dns-lookup-skip.js
	test/parallel/test-repl-mode.js
	test/parallel/test-repl-options.js
	test/parallel/test-repl-tab.js
	test/parallel/test-util-inspect.js
@Fishrock123
tls: make server not use DHE in less than 1024bits
f8e5eaa 
DHE key lengths less than 1024bits is already weaken as pointed out in
https://weakdh.org/ . 1024bits will not be safe in near future. We
will extend this up to 2048bits somedays later.

PR-URL: nodejs#1739
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Fedor Indutny <fedor@indutny.com>
@ChALkeR @Fishrock123
util: speed up common case of formatting string
0c6a548 
PR-URL: nodejs#1749
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
@jbergstroem @Fishrock123
build: refactor pkg-config for shared libraries
cb3326f 
Improve detection and usage of pkg-config. This simplifies the setup
of all our shared libraries.

If pkg-config is installed on the host and `--shared` flags are passed
by the user, we try to get defaults from pkg-config instead of using the
default provided by configure.

PR-URL: nodejs#1603
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
@Fishrock123
Copy link
Contributor Author

Updated, @rvagg lgty? I'll propose 1.8.3 after this lands.

@skenqbx skenqbx mentioned this pull request May 22, 2015
Closed
@rvagg
Copy link
Member

rvagg commented May 23, 2015

I feel this may be reaching too far for an LTS, there's a lot of risk in some of these changes and I think my preference would be to focus on fixes more than performance improvements—note that the async listener changes are both semver-minor and semver-major but haven't been labelled as such as they are undocumented and I guess "experimental", so I'd rather not see them make it in.

We need a better philosophical approach to defining LTS so we can tighten the scope of what gets in; I think this may be too much. Particularly in lieu of a full LTS WG that can comment on all of this I think we should opt for greater conservatism.

I'll try and make time within the next couple of days to go over the commit list and comment and see if I can better define my thoughts about this through the process.

@Fishrock123
Copy link
Contributor Author

Right, that's fine, let me know what should be dropped. :)

@Starefossen Starefossen mentioned this pull request Jun 17, 2015
Closed
@rvagg rvagg mentioned this pull request Jun 29, 2015
Closed
@Fishrock123 Fishrock123 mentioned this pull request Jul 3, 2015
Closed
@ChALkeR
Copy link
Member

ChALkeR commented Aug 17, 2015

Is this still needed?

@Fishrock123 Fishrock123 removed the v1.x label Aug 17, 2015
@Fishrock123
Copy link
Contributor Author

Closing unless we still want to patch up 1.x

@Fishrock123 Fishrock123 mentioned this pull request Aug 17, 2015
Closed
@rvagg
Copy link
Member

rvagg commented Aug 18, 2015

let it go

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.