Skip to content

tls: re-allow falsey option values#15131

Closed
addaleax wants to merge 1 commit into
nodejs:masterfrom
addaleax:ca-null
Closed

tls: re-allow falsey option values#15131
addaleax wants to merge 1 commit into
nodejs:masterfrom
addaleax:ca-null

Conversation

@addaleax
Copy link
Copy Markdown
Member

@addaleax addaleax commented Sep 1, 2017

5723c4c was an unintentional breaking change in that it changed the behaviour of tls.createSecureContext() to throw on false-y input rather than ignoring it. This breaks real-world applications like npm.

This restores the previous behaviour.

Ref: #15053

/cc @mscdex

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • commit message follows commit guidelines
Affected core subsystem(s)

tls

5723c4c was an unintentional breaking change in that it changed
the behaviour of `tls.createSecureContext()` to throw on false-y input
rather than ignoring it. This breaks real-world applications like `npm`.

This restores the previous behaviour.

Ref: nodejs#15053
@nodejs-github-bot nodejs-github-bot added the tls Issues and PRs related to the tls subsystem. label Sep 1, 2017
@addaleax addaleax mentioned this pull request Sep 1, 2017
2 tasks
@addaleax
Copy link
Copy Markdown
Member Author

addaleax commented Sep 1, 2017

@jasnell
Copy link
Copy Markdown
Member

jasnell commented Sep 1, 2017

Good catch

@mscdex
Copy link
Copy Markdown
Contributor

mscdex commented Sep 1, 2017

LGTM. FWIW the only reason I added those explicit checks was for TurboFan, which prefers more explicit comparisons.

@mscdex mscdex mentioned this pull request Sep 5, 2017
4 tasks
Copy link
Copy Markdown
Member

@mhdawson mhdawson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@mhdawson
Copy link
Copy Markdown
Member

mhdawson commented Sep 5, 2017

CI run run OSX machine seemed to be offline for last run: https://ci.nodejs.org/job/node-test-pull-request/9947/

@sam-github
Copy link
Copy Markdown
Contributor

Is this close to landing? Node master looks like its been unable to run npm for 4 days, it might be better to revert #15053

@mhdawson
Copy link
Copy Markdown
Member

mhdawson commented Sep 5, 2017

In last ci run:

OSX failure -> async-hooks/test-callback-error

arm failure is build related.

OSX failure

not ok 76 async-hooks/test-callback-error
  ---
  duration_ms: 15.551
  severity: fail
  stack: |-
    start case 1
    end case 1: 114.761ms
    start case 2
    end case 2: 115.167ms
    start case 3
    end case 3: 8.221ms
    Error: test_callback_abort
        at ActivityCollector.initHooks.oninit.common.mustCall (/Users/iojs/build/workspace/node-test-commit-osx/nodes/osx1010/test/async-hooks/test-callback-error.js:36:45)
        at ActivityCollector.oninit (/Users/iojs/build/workspace/node-test-commit-osx/nodes/osx1010/test/common/index.js:509:15)
        at ActivityCollector._init (/Users/iojs/build/workspace/node-test-commit-osx/nodes/osx1010/test/async-hooks/init-hooks.js:182:10)
        at emitInitNative (async_hooks.js:446:43)
        at Object.emitInitScript [as emitInit] (async_hooks.js:349:3)
        at Object.<anonymous> (/Users/iojs/build/workspace/node-test-commit-osx/nodes/osx1010/test/async-hooks/test-callback-error.js:38:17)
        at Module._compile (module.js:549:30)
        at Object.Module._extensions..js (module.js:560:10)
        at Module.load (module.js:483:32)
        at tryModuleLoad (module.js:446:12)
     1: node::Abort() [/Users/iojs/build/workspace/node-test-commit-osx/nodes/osx1010/out/Release/node]
     2: node::Chdir(v8::FunctionCallbackInfo<v8::Value> const&) [/Users/iojs/build/workspace/node-test-commit-osx/nodes/osx1010/out/Release/node]
     3: v8::internal::FunctionCallbackArguments::Call(void (*)(v8::FunctionCallbackInfo<v8::Value> const&)) [/Users/iojs/build/workspace/node-test-commit-osx/nodes/osx1010/out/Release/node]
     4: v8::internal::MaybeHandle<v8::internal::Object> v8::internal::(anonymous namespace)::HandleApiCallHelper<false>(v8::internal::Isolate*, v8::internal::Handle<v8::internal::HeapObject>, v8::internal::Handle<v8::internal::HeapObject>, v8::internal::Handle<v8::internal::FunctionTemplateInfo>, v8::internal::Handle<v8::internal::Object>, v8::internal::BuiltinArguments) [/Users/iojs/build/workspace/node-test-commit-osx/nodes/osx1010/out/Release/node]
     5: v8::internal::Builtin_Impl_HandleApiCall(v8::internal::BuiltinArguments, v8::internal::Isolate*) [/Users/iojs/build/workspace/node-test-commit-osx/nodes/osx1010/out/Release/node]
     6: 0x31be80046fd
    
  ...
@mhdawson
Copy link
Copy Markdown
Member

mhdawson commented Sep 5, 2017

Looking at the failing test, does not seem related to tls so I don't think it is related to this failure. Opened #15208

@refack
Copy link
Copy Markdown
Contributor

refack commented Sep 5, 2017

OSX failure -> async-hooks/test-callback-error

It's unrelated, it shows up in all latest builds (caused by the macOS VM rebuilt)

@mhdawson
Copy link
Copy Markdown
Member

mhdawson commented Sep 5, 2017

ok landing change now.

@mhdawson
Copy link
Copy Markdown
Member

mhdawson commented Sep 5, 2017

Landed as 1403d28

@mhdawson mhdawson closed this Sep 5, 2017
mhdawson pushed a commit that referenced this pull request Sep 5, 2017
5723c4c was an unintentional breaking change in that it changed
the behaviour of `tls.createSecureContext()` to throw on false-y input
rather than ignoring it. This breaks real-world applications like `npm`.

This restores the previous behaviour.

PR-URL: #15131
Ref: #15053
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Alexey Orlenko <eaglexrlnk@gmail.com>
Reviewed-By: MichaëZasso <targos@protonmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: Timothy Gu <timothygu99@gmail.com>
Reviewed-By: Brian White <mscdex@mscdex.net>
@MylesBorins
Copy link
Copy Markdown
Contributor

as the original didn't land I'm markign this dont-land as well

please lmk if this is a mistake

@addaleax addaleax deleted the ca-null branch September 10, 2017 18:34
addaleax added a commit to addaleax/node that referenced this pull request Sep 13, 2017
5723c4c was an unintentional breaking change in that it changed
the behaviour of `tls.createSecureContext()` to throw on false-y input
rather than ignoring it. This breaks real-world applications like `npm`.

This restores the previous behaviour.

PR-URL: nodejs#15131
Ref: nodejs#15053
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Alexey Orlenko <eaglexrlnk@gmail.com>
Reviewed-By: MichaëZasso <targos@protonmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: Timothy Gu <timothygu99@gmail.com>
Reviewed-By: Brian White <mscdex@mscdex.net>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

tls Issues and PRs related to the tls subsystem.