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

Backport 7373: vm: fix nested timeouts with inverse order + fix flaky test-vm-timeout #7667

Closed
wants to merge 47 commits into from

Conversation

addaleax
Copy link
Member

Cherry-picked first and last commit from #7373

@addaleax addaleax added vm Issues and PRs related to the vm subsystem. test Issues and PRs related to the tests. v4.x labels Jul 11, 2016
@MylesBorins
Copy link
Contributor

bnoordhuis and others added 24 commits July 11, 2016 22:33
Add a basic regression test that checks if the map for IncomingMessage
and OutgoingMessage objects is stable over time.

The test is not exhaustive in that it doesn't try to establish whether
the transition path is the same on every request, it just checks that
objects in their final states have the same map.

To be investigated why the first (and only the first) ServerRequest
object ends up with a deprecated map, regardless of the number of
iterations.

PR-URL: nodejs#7003
Refs: nodejs#6294
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Fixes: nodejs#6925
PR-URL: nodejs#7270
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Refs: nodejs#6655

PR-URL: nodejs#6694
Reviewed-By: Myles Borins <myles.borins@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Ingvar Stepanyan <me@rreverser.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
PR-URL: nodejs#7407
PR-URL: nodejs#7320
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Rod Vagg <rod@vagg.org>
PR-URL: nodejs#7364
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
Clarify how the encoding option interacts with the data
type of child process stdout and stderr.

Fixes: nodejs#6666
PR-URL: nodejs#7361
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Fix typo in example

PR-URL: nodejs#7411
Reviewed-By: Brian White <mscdex@mscdex.net>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Remove special handling when asserting on a pair of arguments objects.
The code being removed will only run if both `expected` and `actual` are
arguments objects. Given that situation, the subsequent code for
handling everything else works just fine.

Tests added to confirm expected behavior.

This came about while trying to improve test coverage. The segment of
code removed had no test coverage. I was unable to write a test that
would both exercise the code and fail if the code was removed. Further
examination indicated that this was because the special handling was not
needed.

PR-URL: nodejs#7413
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
PR-URL: nodejs#7466
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Bryan English <bryan@bryanenglish.com>
Reviewed-By: Brian White <mscdex@mscdex.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
PR-URL: nodejs#7467
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Brian White <mscdex@mscdex.net>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Santiago Gimeno <santiago.gimeno@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
The example changed by this commit uses ['ignore'] where
'ignore' is intended.

Fixes: nodejs#7269
PR-URL: nodejs#7540
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: João Reis <reis@janeasystems.com>
PR-URL: nodejs#7567
PR-URL: nodejs#7528
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Increase socket timeout so that there is enough time to reliably run the
test on FreeBSD.

Fixes: nodejs#7516
PR-URL: nodejs#7555
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Santiago Gimeno <santiago.gimeno@gmail.com>
Added clarification about the linter execution.

PR-URL: nodejs#7534
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Add benchmark to "Who to CC". Also, alphabetized the only
non-alphabetized subsystem.

PR-URL: nodejs#7604
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
The dns.resolve documentation stated that an array of IP
addresses would be returned in the callback. This is true
for everything other than the SOA record which returns an object.
This fixes that documentation.

Fixes: nodejs#6506
PR-URL: nodejs#7532
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Roman Reiss <me@silverwind.io>
`oldDirs` is assigned but never used. Remove it.

(This was missed by the linter in previous versions of ESLint but is
flagged by the current version. Updating the linter is contingent on
this change or some similar remedy landing.)

PR-URL: nodejs#7594
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Michaël Zasso <mic.besace@gmail.com>
`connections` is assigned but never used. Remove it.

(This was missed by the linter in previous versions of ESLint but is
flagged by the current version. Updating the linter is contingent on
this change or some similar remedy landing.)

PR-URL: nodejs#7595
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
`messageCount` is assigned, but never used. Remove it.

(This was missed by the linter in previous versions of ESLint but is
flagged by the current version. Updating the linter is contingent on
this change or some similar remedy landing.)

PR-URL: nodejs#7599
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Michaël Zasso <mic.besace@gmail.com>
`writes` is assigned but never used. Remove it.

(This was missed by the linter in previous versions of ESLint but is
flagged by the current version. Updating the linter is contingent on
this change or some similar remedy landing.)

PR-URL: nodejs#7596
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Michaël Zasso <mic.besace@gmail.com>
`connections` is assigned but never used. Remove it.

(This was missed by the linter in previous versions of ESLint but is
flagged by the current version. Updating the linter is contingent on
this change or some similar remedy landing.)

PR-URL: nodejs#7597
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Michaël Zasso <mic.besace@gmail.com>
Remove variables that are assigned but never used.

(This was missed by the linter in previous versions of ESLint but is
flagged by the current version. Updating the linter is contingent on
this change or some similar remedy landing.)

PR-URL: nodejs#7600
Reviewed-By: Rod Vagg <rod@vagg.org>
Reviewed-By: Michaël Zasso <mic.besace@gmail.com>
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
Trott and others added 18 commits July 11, 2016 22:33
Update ESLint to 3.0.0. This includes an enhancement to `no-unused-vars`
such that it finds a few instances in our code base that it did not find
previously (fixed in previous commits readying this for landing).

PR-URL: nodejs#7601
Reviewed-By: Michaël Zasso <mic.besace@gmail.com>
Reviewed-By: Roman Reiss <me@silverwind.io>
Previous version of weak used for gc tests emitted a warning on OS X.
Updating to current version eliminates warning.

PR-URL: nodejs#7014
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: James M Snell <jasnell@gmail.com>
Code coverage information shows that we are only testing the happy path
for the internal readline `isFullWidthCodePoint()` function. Test it
with invalid input.

PR-URL: nodejs#7422
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Brian White <mscdex@mscdex.net>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: James M Snell <jasnell@gmail.com>
This commit adds the missing handle argument to the cluster
worker 'message' event. It also adds a link to the process
'message' event for reference.

Refs: nodejs#7297
PR-URL: nodejs#7309
Reviewed-By: Brian White <mscdex@mscdex.net>
strcasecmp() is not used in src/node_http_parser.cc so there is no need
to include its header file.

PR-URL: nodejs#6582
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Fedor Indutny <fedor.indutny@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
strcasecmp() is affected by the current locale as configured through
e.g. the LC_ALL environment variable and the setlocale() libc function.

It can result in unpredictable results across systems so replace it with
a function that isn't susceptible to that.

PR-URL: nodejs#6582
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Fedor Indutny <fedor.indutny@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
This is the certdata.txt[0] that ships in Firefox 47 and NSS 3.23, last
updated on 2016-02-26.

[0] https://hg.mozilla.org/mozilla-central/raw-file/1f84dea6508d/security/nss/lib/ckfw/builtins/certdata.txt

PR-URL: nodejs#7363
Reviewed-By: Fedor Indutny <fedor.indutny@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
Update the list of root certificates in src/node_root_certs.h with
tools/mk-ca-bundle.pl.

Certificates added:
- CA WoSign ECC Root
- Certification Authority of WoSign G2
- Certinomis - Root CA
- Certum Trusted Network CA 2
- OISTE WISeKey Global Root GB CA
- SZAFIR ROOT CA2
- TURKTRUST Elektronik Sertifika Hizmet Sa?layıcısı H5
- TURKTRUST Elektronik Sertifika Hizmet Sa?layıcısı H6

Certificates removed:
- A-Trust-nual-03
- Buypass Class 3 CA 1
- CA Disig
- ComSign Secured CA
- Equifax Secure CA
- NetLock Notary (Class A) Root
- Staat der Nederlanden Root CA
- TC TrustCenter Class 2 CA II
- TC TrustCenter Universal CA I
- TURKTRUST Certificate Services Provider Root 1
- TURKTRUST Certificate Services Provider Root 2
- UTN DATACorp SGC Root CA
- Verisign Class 4 Public Primary Certification Authority - G3

PR-URL: nodejs#7363
Reviewed-By: Fedor Indutny <fedor.indutny@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
Shigeki Ohtsu points out that the test is unreliable because some of
the www1.cnnnic.cn servers are misconfigured.  Remove it.

PR-URL: nodejs#7363
Refs: nodejs#7363 (comment)
Reviewed-By: Fedor Indutny <fedor.indutny@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
Pointed out by Coverity.

PR-URL: nodejs#7374
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
This commit adds a CHECK that verifies that the file event watcher is
not started twice, which would be indicative of a bug in lib/fs.js.

PR-URL: nodejs#7374
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Remove TLSWrap::write_queue_size_, it's not used anywhere.

PR-URL: nodejs#7374
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
The code assigned the result of EVP_get_digestbyname() to data members
called md_ that were not used outside the initialization functions.

PR-URL: nodejs#7374
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
PR-URL: nodejs#7374
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
PR-URL: nodejs#7374
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Pointed out by Coverity.  Introduced in commit 5b8e1da from September
2011 ("Initial pass at zlib bindings".)

The asynchronous version of Write() used a pointer to a stack-allocated
buffer on flush.  A mitigating factor is that zlib does not dereference
the pointer for zero-sized writes but it's still technically UB.

PR-URL: nodejs#7374
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Pointed out by Coverity.  Introduced in commits 3546383 ("process_wrap:
avoid leaking memory when throwing due to invalid arguments") and
fa4eb47 ("bindings: add spawn_sync bindings").

The return statements inside the if blocks were dead code because their
guard conditions always evaluated to false.  Remove them.

PR-URL: nodejs#7374
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Catch and emit `certCbDone` exceptions instead of throwing them as
`uncaughtException` and crashing the whole process.

Fix: nodejs#6822
PR-URL: nodejs#6887
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
targos and others added 4 commits July 12, 2016 08:29
The test targets expect that V8 is built in deps/v8/out

Ref: nodejs#7477
PR-URL: nodejs#7482
Reviewed-By: Myles Borins <myles.borins@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Without this it would always compile Release and Debug builds.

Ref: nodejs#7477
PR-URL: nodejs#7482
Reviewed-By: Myles Borins <myles.borins@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Likely fix the flaky parallel/test-vm-timeout. Increase the outer
timeout in the test checking for nested timeouts with `vm` scripts
so that its firing won’t interfere with the inner timeout.

Fixes: nodejs#6727
PR-URL: nodejs#7373
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
This reverts commit f34caa9.

PR-URL: nodejs#7373
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
@MylesBorins
Copy link
Contributor

landed in 6aa1511...23a5164

sorry again for the mangled history in here... rebase hell 😄

@addaleax
Copy link
Member Author

I think this can be closed then. :)

@addaleax addaleax closed this Jul 12, 2016
@addaleax addaleax deleted the backport-7373 branch July 12, 2016 17:30
@MylesBorins MylesBorins removed their assignment Dec 27, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
test Issues and PRs related to the tests. vm Issues and PRs related to the vm subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.