From 9e4f94bf2b988c2cc9f86404e6ff473b94e4aa36 Mon Sep 17 00:00:00 2001 From: Rod Vagg Date: Wed, 3 Feb 2016 22:24:29 +1100 Subject: [PATCH 01/15] doc: add CTC meeting minutes 2016-01-27 PR-URL: https://github.com/nodejs/node/pull/5057 Reviewed-By: Roman Reiss Reviewed-By: Ben Noordhuis Reviewed-By: Sakthipriyan Vairamani Reviewed-By: James M Snell --- doc/ctc-meetings/2016-01-27.md | 295 +++++++++++++++++++++++++++++++++ 1 file changed, 295 insertions(+) create mode 100644 doc/ctc-meetings/2016-01-27.md diff --git a/doc/ctc-meetings/2016-01-27.md b/doc/ctc-meetings/2016-01-27.md new file mode 100644 index 00000000000000..3e185c973babfd --- /dev/null +++ b/doc/ctc-meetings/2016-01-27.md @@ -0,0 +1,295 @@ +# Node Foundation CTC Meeting 2016-01-27 + +## Links + +* **Audio Recording**: https://soundcloud.com/node-foundation/ctc-meeting-2016-01-27 +* **GitHub Issue**: https://github.com/nodejs/node/issues/4901 +* **Minutes Google Doc**: +* _Previous Minutes Google Doc: _ + +## Present + +* James Snell (CTC) +* Trevor Norris (CTC) +* Colin Ihrig (CTC) +* Brian White (CTC) +* Alexis Campailla (CTC) +* Bert Belder (CTC) (absent) +* Chris Dickinson (CTC) +* Shigeki Ohtsu (CTC) (absent) +* Steven Loomis (observer) (absent) +* Mikeal Rogers (observer) +* Fedor Indutny (CTC) +* Jeremiah Senkpiel (CTC) +* Rod Vagg (CTC) +* Ben Noordhuis (CTC) +* Domenic Denicola (observer) (absent) +* Nikita Skovoroda (observer) +* Ali Sheikh (observer) (absent) +* Evan Lucas (observer) +* Rich Trott (observer) +* Michael Dawson (observer) + + +## Agenda + +Extracted from **ctc-agenda** labelled issues and pull requests from the **nodejs org** prior to the meeting. + +* Enable Node.js to run with Microsoft's ChakraCore engine [#4765](https://github.com/nodejs/node/pull/4765) +* CTC Membership Nominations [#4750](https://github.com/nodejs/node/issues/4750) +* buffer: add Buffer.from(), Buffer.alloc() and Buffer.allocUnsafe(), soft-deprecate Buffer(num) [#4682](https://github.com/nodejs/node/pull/4682) +* Buffer(number) is unsafe [#4660](https://github.com/nodejs/node/issues/4660) +* doc: add Docs working group [#4244](https://github.com/nodejs/node/pull/4244) +* Seek legal advice on LICENSE and copyright blocks in code [#3979](https://github.com/nodejs/node/issues/3979) +* Drop Windows XP (and Vista) support in 6.0 [#3804](https://github.com/nodejs/node/issues/3804) + +## Standup + +* James Snell (CTC) — Buffer API change proposal, PR review, error handling summit yesterday, community related issues +* Trevor Norris (CTC) — Writing a patch for Buffer#fill, so it can accept an encoding and a buffer; making MakeCallback reentrant; have a fix but not sure how it’ll affect 3p debugger modules; want to be thorough because this should extend back to v4 +* Colin Ihrig (CTC) — Reviewing issues/PRs, fixing bugs; PR landed in libuv for looking up tmpdir, PR into child process +* Brian White (CTC) — Reviewing/commenting on PRs and issues, working on JS optimizations inside core modules. +* Alexis Campailla (CTC) — Issues/PRs, some work in the testing repo, some work in CI, visual C++ build tools for making modules, investigated a couple issues in libuv +* Chris Dickinson (CTC) — docs WG organization +* Mikeal Rogers (observer) — New education initiatives coming up from Foundation board, legal, working on size & scope of TSC stuff and the incubator +* Jeremiah Senkpiel (CTC) — Administrative and TSC stuff; moderating large issues [CD — thank you!] +* Rod Vagg (CTC) — (Possibly catching up on sleep) +* Ben Noordhuis (CTC) — Reviewing libuv+node PRs, reply to issues, 2 months ahead are busy +* Nikita Skovoroda (observer) — some comments/reviews, nothing interesting. Npm code search update. [JS - Thanks for doing the npm search things!] +* Evan Lucas (observer) — Minor dns/net cleanups, Responding to issues/PRs +* Rich Trott (observer) — Tests, spinning up Testing WG, removing redeclaration of vars from code, especially tests, thus reducing side effects and making it easier to split up tests. +* Michael Dawson (observer) — Benchmarks, results of benchmarks on the website, PPC for big endian machines, V8 testing separate from the node install +* Fedor Indutny (CTC) - Reviewing Pull Requests, fixing issues, V8 code cache API +* Seth Thompson (observer) - Google folks replied to James’ scheduling for face to face meeting for talking about swapping out VMs + +## Review of last meeting + + +* Enable Node.js to run with Microsoft's ChakraCore engine [#4765](https://github.com/nodejs/node/pull/4765) +* CTC Membership Nominations [#4750](https://github.com/nodejs/node/issues/4750) +* buffer: add Buffer.from(), Buffer.zalloc() and Buffer.alloc(), soft-deprecate Buffer(num) [#4682](https://github.com/nodejs/node/pull/4682) +* Buffer(number) is unsafe [#4660](https://github.com/nodejs/node/issues/4660) +* util: deprecate `util._extend` [#4593](https://github.com/nodejs/node/pull/4593) +* ArrayBuffer.isView() and buffer.buffer property [#4420](https://github.com/nodejs/node/issues/4420) +* doc: add Docs working group [#4244](https://github.com/nodejs/node/pull/4244) +* Seek legal advice on LICENSE and copyright blocks in code [#3979](https://github.com/nodejs/node/issues/3979) +* fs: optimize realpath using uv_fs_realpath() [#3594](https://github.com/nodejs/node/pull/3594) + + +## Minutes + +### Enable Node.js to run with Microsoft's ChakraCore engine [#4765](https://github.com/nodejs/node/pull/4765) + +Alexis: No action items on the PR itself, which was locked to collabs, since it was turning into noise. Another issue was opened (roadmap#54) by Mikeal re: whether we should be VM agnostic in the future, there’s discussion of pros and cons as well as talk of how this might happen in the future. Chakra folks are on board for the API wg group / VM agnostic API. + +Fedor: Do we have consensus yet? + +Alexis: Not yet. + +Mikeal: The vast majority agree that we should be moving toward it, + +Ben: Is it actually true? I’ve seen it go both ways. + +Mikeal: I haven’t seen objections to being VM neutral, but how we go about it — there’s no agreement at all. + +James: There’s concerns around language support between the two and how that affects us — there’s a lot of issues like that, related to the “how” we go about this, but there seems to be a consensus that this is the way we want to go. + +Mikeal: I’d like to do this before April? + +Ben: Are there collaborators that haven’t spoken up yet, on the principle that they don’t feel too strongly about it? + +Jeremiah: I’m more in line with Oglas (the guy maintaining JXCore), he’s got a lot of experience with this, and his idea was to move to its own VM. I’m not sure we could + +Trevor: You mean maintaining our own VM? + +Mikeal: I had a conversation with Mark Mayo and he thought that would be the direction we’d actually land. + +Alexis: That would require an enormous amount of resources. + +Mikeal: This PR, you say we locked it to collabs, but aren’t the folks posting it not collabs? + +Jeremiah: In the meantime we can add them to a team for this purpose. + +Alexis: I think it looks like the original PR submitter can still comment. + +Alexis: I can facilitate since I’m in contact with them daily. + +Mikeal: We should create teams for V8 and a team for Chakra, for our convenience. + +Alexis: We do have a v8 team, but I think it’s the Node experts on V8. + +Seth: I can make sure the right people from the team are on there. + +Alexis: And I can create one for the Chakra team. + +Michael: there are a few folks I’d like to add from my team, how would I do it. + +Mikeal: James can do it. + +Another question: assuming we do move this direction, should we start an issue around no longer pulling deps into the source tree? + +Michael: Being vm-neutral doesn’t mean we bring in every VM into the source tree? + +Mikeal: But we’re set up to do that right now. + +Alexis: This also affects ICU. + +Jeremiah: I don’t think this is an immediate concern. + +Fedor: Release cycle — Chakra and V8’s release cycle are not aligned, how will that affect us? + +Alexis: Chakra team has folks dedicated to working with Node’s release cycle, so we would align. + +Mikeal: If we tackle this ABI compatibility problem, we may end up revisiting the release cycle since that’s driven by these big VM breaks. + +### CTC Membership Nominations [#4750](https://github.com/nodejs/node/issues/4750) + +Jeremiah: We’re going to keep this open for a couple weeks while the nominees sit in. The vote is TBD. We don’t do this often so the process is rusty. + +### buffer: add Buffer.from(), Buffer.alloc() and Buffer.allocUnsafe(), soft-deprecate Buffer(num) [#4682](https://github.com/nodejs/node/pull/4682) + +James: Where we’re at right now: continuing to update the PR and the EPS proposal that goes along with this based on the conversation. The new APIs seem uncontroversial. Unsurprisingly the name of one method is the most controversial. Currently, it’s allocUnsafe, a few folks don’t like “Unsafe”, would prefer “Raw”. [CD - yay bikesheds!] + +Some of the feedback from Trevor was that the API should be tweaked a bit, where we can pass encoding or a buffer to `.fill`, other than that the conversation around it is dying down. We haven’t decided if we’re going to change the default `new Buffer(n)` to zero-fill — if we do make that change we have to make it all the way down to v0.10 to keep it consistent. + +Mikeal: Call it buffer.remoteDisclosure(). + +Just kidding. + +Ben: if we could make zero-fill as fast as the current impl, then we would of course pick that. + +James: Yes. + +Ben: I’ve been playing with doing init in a separate thread. Perhaps I can close the gap. + +Mikeal: The only objection is perf. + +James: Yep. The baseline impl is that zero fill uses calloc. + +Trevor: We use a pool. It’s always been possible to reach the pool from any buffer instance. Do we want to remove pooling? + +James: Current impl is that zerofill does not pool. + +Trevor: So much overhead! + +_(in document chat)_ + +_Chris: why not use WeakMap for pooling?_ + +_Ben: no weakmaps in v0.10._ + +Jeremiah: Do we have benchmarks for calloc? + +James: Once we get a little further, I’ll revisit benchmarks. + +Trevor: Can the kernel do magic where memory isn’t actually allocated when it’s done in a tight loop? [CD - I didn’t capture this completely] + +Ben: You can mmap memory, memory isn’t actually allocated + zerofilled until you touch it, but touching it causes a page fault. + +### Buffer(number) is unsafe [#4660](https://github.com/nodejs/node/issues/4660) + +Jeremiah: Looks like we covered this. + +### doc: add Docs working group [#4244](https://github.com/nodejs/node/pull/4244) + +Chris: Onboarded some folks. Completed drafting the docs roadmap proposal. Making sure we have adequate tooling to build docs to the website from the node.js core repo. + +Chris: There is a docs wg meeting next week [10AM PST] [Wednesday @ 10am Feb 3rd] + +Chris: Also working with the inclusivity WG to make a new collaborator guide. + +Ben: No loosening of commit formatting, right? + +Chris, Mikeal: nope + +Jeremiah: Has anyone read through the proposed charter? + +Mikeal: hasn’t changed recently + +Ben, James: read though it recently, lgtm + +Jeremiah: Vote? + +Mikeal: No need, no-one objects. + +Jeremiah: Ok, good to go. + +### Seek legal advice on LICENSE and copyright blocks in code [#3979](https://github.com/nodejs/node/issues/3979) + +Mikeal: We’re nearing final documents, and we have some additions to contributing guide that come from legal. The SPDX stuff we talked about. The Foundations IP policy, we mention that we’re going to use CC-4.0 for docs. We should put this to the Docs WG about this. We should have a proper policy from the legal committee really soon. + +Chris: I will bring the CC-4.0 license to the Docs WG. + +### Drop Windows XP (and Vista) support in 6.0 [#3804](https://github.com/nodejs/node/issues/3804) + +Jeremiah: Realpath brought this back up. + +Ben: That was Rod but I can take over. The proposal was to drop vista support in the next major release, that would let us drop a lot of legacy of code. TBH, I thought we had already decided on this, so I’m surprised about this. If you’re not in favor, now would be the time to speak up. + +Trevor: Didn’t libuv drop XP? + +Ben: Nope. + +Trevor: Everything but some subset of the API works on XP? + +Alexis: I also thought this decision was already made. + +Jeremiah: It might have been in some other issue. I seem to recall. + +Alexis: Does anyone object to dropping XP? + +_[crickets]_ + +Fedor: I do! `` + +No objections. + +Alexis: Does anyone object to drop Vista support. + +Mikeal: Let’s get data from NPM. + +Ben: Info was posted on the issue, looks like there’s more XP users than Vista, but still very small percentage. + +Mikeal: Last I checked there were banks that required it, so it’s still out there. + +Trevor: They have LTS, right? + +Colin: If we were going to drop support, that we need to start messaging as soon as possible. + +Alexis: And that we’d put a check so that Node would exit on those platforms. + +Mikeal: It sounds like we’re going to cut some number of users. As long as we’re okay with that… + +Alexis: I think we’re going to support the other users better. + +Trevor: We have v4, and that will be around for [another year? CD] Maybe even V8 won’t support XP by the time v4 expires. + +Jeremiah: I think V8 already stopped supporting it. + +Alexis: I think Nikita articulated that. + +Jeremiah: Seth, if you’re still there, do you have any idea when V8 will drop XP support. + +Seth: We’ve planned that Chrome will probably drop XP support at the same time as … I have to double check. I’ll come back when I find out. + +Mikeal: We’ll drop it, and if anyone complains we’ll just say Microsoft told us to do it :) + +Alexis: I can make the change to stop on startup; in terms of messaging what needs to happen? + +Colin: We just need to get the word out. A tweet or a blogpost or something? + +Jeremiah: We should loop in marketing. + +Seth: It’s April 2016 for sure on the Chrome side. + +Jeremiah: So we have 3 months. + +Mikeal: It’d be a major? + +Jeremiah: Yep. v6. + +Alexis to PR this in. + +## Next Meeting + +2016-02-03 From e325ece23e880322306de641c5af8cd8b0e18c70 Mon Sep 17 00:00:00 2001 From: Alexander Makarenko Date: Sun, 31 Jan 2016 17:38:04 +0300 Subject: [PATCH 02/15] doc: minor improvement in OS docs Add links to `process.arch` and `process.platform`. PR-URL: https://github.com/nodejs/node/pull/5006 Reviewed-By: Roman Klauke Reviewed-By: James M Snell Reviewed-By: Roman Reiss --- doc/api/os.markdown | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/doc/api/os.markdown b/doc/api/os.markdown index 6c1a6f3634e69a..ea4f40ba79e958 100644 --- a/doc/api/os.markdown +++ b/doc/api/os.markdown @@ -14,7 +14,7 @@ system. ## os.arch() Returns the operating system CPU architecture. Possible values are `'x64'`, -`'arm'` and `'ia32'`. Returns the value of `process.arch`. +`'arm'` and `'ia32'`. Returns the value of [`process.arch`][]. ## os.cpus() @@ -159,7 +159,7 @@ interfaces that have been assigned an address. Returns the operating system platform. Possible values are `'darwin'`, `'freebsd'`, `'linux'`, `'sunos'` or `'win32'`. Returns the value of -`process.platform`. +[`process.platform`][]. ## os.release() @@ -181,3 +181,6 @@ on OS X and `'Windows_NT'` on Windows. ## os.uptime() Returns the system uptime in seconds. + +[`process.arch`]: process.html#process_process_arch +[`process.platform`]: process.html#process_process_platform From 28b392854cdfc76130f6f3ac8a05ac5bbbf7d8ca Mon Sep 17 00:00:00 2001 From: Rod Vagg Date: Thu, 28 Jan 2016 00:20:11 +1100 Subject: [PATCH 03/15] doc: add CTC meeting minutes 2016-01-20 PR-URL: https://github.com/nodejs/node/pull/4904 Reviewed-By: James M Snell Reviewed-By: Ben Noordhuis Reviewed-By: Roman Reiss --- doc/ctc-meetings/2016-01-20.md | 337 +++++++++++++++++++++++++++++++++ 1 file changed, 337 insertions(+) create mode 100644 doc/ctc-meetings/2016-01-20.md diff --git a/doc/ctc-meetings/2016-01-20.md b/doc/ctc-meetings/2016-01-20.md new file mode 100644 index 00000000000000..ceffa500b103b6 --- /dev/null +++ b/doc/ctc-meetings/2016-01-20.md @@ -0,0 +1,337 @@ +# Node Foundation CTC Meeting 2016-01-20 + +## Links + +* **Audio Recording**: https://soundcloud.com/node-foundation/ctc-meeting-2016-01-20 +* **GitHub Issue**: https://github.com/nodejs/node/issues/4780 +* **Minutes Google Doc**: +* _Previous Minutes Google Doc: _ + +## Present + +* James Snell (CTC) +* Trevor Norris (CTC) +* Colin Ihrig (CTC) +* Brian White (CTC) +* Alexis Campailla (CTC) +* Bert Belder (CTC) +* Chris Dickinson (CTC) +* Shigeki Ohtsu (CTC) +* Steven Loomis (observer) +* Mikeal Rogers (observer) +* Jeremiah Senkpiel (CTC) +* Rod Vagg (CTC) +* Ben Noordhuis (CTC) +* Domenic Denicola (observer) +* Nikita Skovoroda (observer) +* Ali Sheikh (observer) +* Evan Lucas (observer) + +## Agenda + +Extracted from **ctc-agenda** labelled issues and pull requests from the **nodejs org** prior to the meeting. + +### nodejs/node + +* Enable Node.js to run with Microsoft's ChakraCore engine [#4765](https://github.com/nodejs/node/pull/4765) +* CTC Membership Nominations [#4750](https://github.com/nodejs/node/issues/4750) +* buffer: add Buffer.from(), Buffer.zalloc() and Buffer.alloc(), soft-deprecate Buffer(num) [#4682](https://github.com/nodejs/node/pull/4682) +* Buffer(number) is unsafe [#4660](https://github.com/nodejs/node/issues/4660) +* util: deprecate `util._extend` [#4593](https://github.com/nodejs/node/pull/4593) +* ArrayBuffer.isView() and buffer.buffer property [#4420](https://github.com/nodejs/node/issues/4420) +* doc: add Docs working group [#4244](https://github.com/nodejs/node/pull/4244) +* Seek legal advice on LICENSE and copyright blocks in code [#3979](https://github.com/nodejs/node/issues/3979) +* fs: optimize realpath using uv_fs_realpath() [#3594](https://github.com/nodejs/node/pull/3594) + +## Standup + +* James Snell (CTC): Working on the Buffer issue, work on HTTP module, reviewing issues and PRs, attempting to help on 4.2.5, +* Trevor Norris (CTC): AsyncWrap related things +* Colin Ihrig (CTC): Reviewing issues and PRs. Reverted a commit +* Brian White (CTC): Not much, commenting on PRs and issues. +* Alexis Campailla (CTC): Fixing Windows issues on libuv and npm +* Bert Belder (CTC): _[CD - missed what you were looking into due to cross talk]_ +* Mikeal Rogers (observer): Various projects coming into the Foundation +* Jeremiah Senkpiel (CTC): Onboarding new release team members, Working on Timers Refactor more, trying to make sure Chakra and Buffer discussions remain productive +* Rod Vagg (CTC): Working with new contributors, lots of meetings +* Chris Dickinson (CTC): Docs WG stuff +* Shigeki Ohtsu (CTC): +* Steven Loomis (observer): +* Ben Noordhuis (CTC): Been sick, still catching up. +* Rich Trott (observer): Making tests more reliable, spinning up Testing WG +* Nikita Skovoroda “Chalker” (observer): mostly buffer, also the api usage greps are now sorted based on downloads count +* Ali Sheikh (observer): sampling heap profiler in V8 +* Domenic (observer): modules and zones +* Evan Lucas (observer): Working on getting v5.5.0 Release out and looking into v8 extras for some of the builtins. + +## Review of last meeting + +* Nominating new Release team members [#4319](https://github.com/nodejs/node/issues/4319) +* repl: Reverses order of .node_repl_history [#4313](https://github.com/nodejs/node/pull/4313) +* doc: add Docs working group [#4244](https://github.com/nodejs/node/pull/4244) +* Seek legal advice on LICENSE and copyright blocks in code [#3979](https://github.com/nodejs/node/issues/3979) +* Potential Licensing issues with npm [#3959](https://github.com/nodejs/node/issues/3959) +* Joyent Copyright still in header of many files [#3926](https://github.com/nodejs/node/issues/3926) +* gripe: deprecating fs.exists/existsSync [#1592](https://github.com/nodejs/node/issues/1592) + +## Minutes + +### Enable Node.js to run with Microsoft's ChakraCore engine [#4765](https://github.com/nodejs/node/pull/4765) + +Put on agenda by Alexis + +Alexis: MS submitted a PR for supporting Chakracore, change in node is fairly small — the shim is in the deps folder. A few CTC members have expressed interest in landing. I was wondering if anyone had opinions about how to proceed with this? + +Domenic: I’m curious about what level of support is implied. Landing it reduces the requirement of maintaining a separate fork, but + +Domenic: What’s the CTC’s goal in merging it in? + +Alexis: Adds Windows IoT. + +Domenic: but it doesn’t, because we’re not supporting it. + +Alexis: I still can build it myself. + +James: As for “why would we land it” — if this is something we think we will eventually support, landing it sends a clear signal that this is the direction we are heading. Landing it now lets the community start contributing to it, and to figure out how + +Trevor: If it was landed, we would not be providing official builds on the website. + +Jeremiah: Maybe Canary builds? + +Mikeal: Microsoft is already providing builds of this. By bringing this into the mainline, the changes get reviewed by the Core team. Its a matter of are those builds off of Node core mainline, or + +Jeremiah: Have we decided we want to this? + +Rod: We’re making a lot of assumptions. + +Trevor: Could we start an issue and list concerns? + +Domenic: Would it be a good idea to restrict to collaborators? + +Jeremiah: There’s not enough outside noise. + +Trevor: Well, the PR has garnered over 100 comments in 24 hours. + +Jeremiah: But it’s not spam. + +Mikeal: What is the title of that issue? + +Domenic: “What is the CTC’s decision on whether to merge the ChakraCore PR?” + +Trevor: or, “Things that prevent it from landing” — if we would hold back a V8 version due to incompability with the shim. James mentioned some concerns as well. + +Jeremiah: How are we going to support a shimmed VM — not officially, but for ourselves in the codebase, when we’re still using a V8 api and exposing that. + +Rod: that’s just one of the questions. + +Mikeal: No one is ever going to say “let’s support chakra” until it’s been in the codebase for a while, and has seen the MS folks show up and support it. + +Rod: We still need to get everyone on board that we want to head in that direction. I don’t think it’s fair to say “get it in the codebase and we’ll sort it out”. I see a lot of assumptions in the thread, and we need to air those assumptions and discuss them + +Mikeal: Is the discussion, “do we want to move in a VM agnostic direction?” Can we scope this? + +Domenic: My worry is: I don’t think the CTC might want to imply full VM agnosticity — they might not want to accept a spidermonkey PR? Maybe making it smaller would makemaek it easier. + +Bert: I agree with Rod that we haven’t actually had the discussion. I’ve always thought it was cool, and pushed for it, but I think _[CD ...]_ +What’s frustrating is that someone has done all the work to land a VM, but we kind of expect it to be “more than perfect” before we land it. I would suggest landing it first but not including it as a default, so we can do comparisons. + +Trevor: don’t feel too bad about this — they had to do this to support IoT. +Bert: I don’t feel bad; are we going to build a higher and higher wall until + +Rod: the problem is that we’re not on the same page, and we need to get on the same page. I’m not hearing anyone say outright that they don’t want VM agnostic. But we’re all saying different things. + +James: I agree. I don’t think there should be any rush in getting the PR landed. We can do some of that review process and take our time over the next few weeks to figure out if this is what we want to do. I don’t think anyone is saying “land it now!” We can take our time. + +Rod: My suggestion is to move back to the issue and let it evolve, and suss out the main philosophical issues from there. There’s so many things to talk about in that thread, and everyone’s g +ot a different take. We’ve got to summarize, and take our time. + +### CTC Membership Nominations [#4750](https://github.com/nodejs/node/issues/4750) + +Rod: A vote will be held in a few weeks time. In the meantime we’ll be in observer mode for these additional people. Not much to discuss here other than, “welcome!” + +### buffer: add Buffer.from(), Buffer.zalloc() and Buffer.alloc(), soft-deprecate Buffer(num) [#4682](https://github.com/nodejs/node/pull/4682) +### Buffer(number) is unsafe [#4660](https://github.com/nodejs/node/issues/4660) + +Rod: want to take it away, James? + +James: Sure everyone’s familiar with the issue up to this point. `Buffer` constructor (`Buffer(num)` or `Buffer(value)` with overloaded). Some portion of the community saying that needs changed, some real world examples of security flaws in the wild, community consensus seems to be that the API should be cleaned up — introducing new factory methods, + +Trevor: Careful saying “community consensus” + +James: Yeah — yes, the discussion seems to lead to where 4682 is now, which has been created as an EPS issue as well (https://github.com/nodejs/node-eps/pull/4). The basic idea is to introduce `Buffer.{from,alloc,allocUnsafe}()`. `allocUnsafe()` would be our current behavior for `Buffer(num)`, alloc would be zero-filled, `.from` takes the place of `Buffer(value)`. Deprecation of constructor would be docs-only, with _possibly_ a warning printed, similar to the memory leak warning on event emitter listeners. The documentation updates go along with that. That’s the overview of the discussion. _[CD phew!]_ + +Bert: What is the argument for not zeroing out buffers, exactly? why do we want to keep supporting that behavior? + +James: comes down to perf + +Trevor: That’s it. You don’t always need to zero fill. + +Domenic: I tried to read through the threads, last I saw there were no benchmarks that were completely fair. `calloc` vs. `memset`, etc. Have there been updated benchmarks? + +Trevor: if the allocation is small enough that it can use the pool, it’s not going to have perf degradation when it becomes larger. + +Domenic: My understanding with larger values is that with calloc makes it free. + +Trevor: No — over 1-4kb. + +Domenic: I don’t know what size “large” is supposed to be for calloc. + +Trevor: I’ve tried for 1mb; perf penalty is 2-3x slower, depending on size. From a dozen to a couple hundred microseconds. + +Domenic: That kind of argues to me for “safe by default.” + +Trevor: I can read in 100’s and 100’s of JSON files all of which are large, I can use fs.read with a buffer, so you have to allocate a buffer, if I have to zerofill it when I’m just going to fill it anyway, that’s just wasted cycle. + +Bert: I think most of the time that will not be the bottleneck. We might have an internal api… + +Trevor: No — the perf penalty on that is so big. This is why I initially got rid of the pooling after my first rewrite. + +Bert: optimization doesn’t have to be done right away. If you’re allocating large buffers, you’re usually mmaping it, so you don’t have to zero fill. + +Ben: But the OS is zeroing it out. + +Bert: So we don’t have to. + +Ben: There’s still CPU time being used there — it’s not free. + +James: The PR has three methods: _[CD — missed the first part]_ `alloc` will do `calloc` under the covers. If a 2nd param is passed, it will do `allocUnsafe` and fill with the string. We can compare all of these using the PR. The current implementation alloc can be 30-60% slower than `allocUnsafe`. + +Ben: We can farm out allocations to another thread, the other thread might need to overalloc a little just to have a bit in reserve. + +Trevor: At minimum that would work for the buffer pool, which is a set size anyway. + +Ben: Is anyone volunteering to write that code? + +James: I already have my hands in there. I need to look into the native buffer constructor anyway. I’ll bring it back to the discussion when I get there. + +Bert: that sounds great. I’m very happy that some experimentation and benchmarking is going on. I’m much in favor of a simple api that is safe by default. Adding a bunch of stuff — like the deprecation in the docs — we are doing too much to deal with not a very complicated issue. + +Mikeal: If you look at all of the exploits, just zero filling doesn’t solve them — they’re still a DoS vector, but no longer a disclosure. WE need the from and alloc APIs to avoid these + +Domenic: that’s a good point. I think adding more explicit APIs would help. + +James: Thank you for pointing that out — it’s an API usability issue. If we decide to zero fill by default this becomes much easier to figure out and we don’t have as much surface area to expand. There is an LTS concern with that — if we switch the default then we run into an issue where we open up a security problem, where modules assume buffer zero fills by default when it doesn’t on the current node version. I’m fine with zero-fill by default. + +Domenic: Real world apps don’t run into perf problems with this quite so often. + +Rod: What do we need to do to move forward? + +James: Ideas on speeding up initialization. Comment on the thread and we’ll go from there. + +### util: deprecate `util._extend` [#4593](https://github.com/nodejs/node/pull/4593) + +Rod: A bunch of +1’s and -1’s. + +Jeremiah: `util._extend` was added Object.assign wasn’t a thing back when we needed it, but since it was publicly exposed, all kinds of folks have used this thing and it’s in wide use. We’re discussing what we want to do with this now that Object.assign is a thing — phase out it’s use? + +Trevor: Is object.assign able to be swapped in? + +Domenic: 95% sure that is the case. + +Trevor: Could we deprecate `_extends` and change it to do Object.assign for the user. + +Domenic: If you overrode object.keys it would give different results. + +Jeremiah: `_extend` is not documented anyway, so for folks to move off of it we’d have to add docs for it or add a deprecation warning + +Rod: my concern: It’s in wide use, it’s undocumented — it just seems like a purist thing to me. “Stop using core stuff!” Apparently Object.assign is a bit slower than extend for our use in core, anyway. I just don’t see any justification for this. + +Domenic: I tend to agree. On the web we’ve got all of this stuff “webkitMatchesSelector” – better to just document it. + +Trevor: I guess what I’m getting at is that there are philosophical issues in trying to + +Document deprecation, but don’t warn on use. + +Domenic is writing a list of minor differences: https://github.com/nodejs/node/pull/4593#issuecomment-173357276 + +### ArrayBuffer.isView() and buffer.buffer property [#4420](https://github.com/nodejs/node/issues/4420) + +Trevor: now that buffer inherits from uint8array, there are undocumented methods on it. If we document that buffer extends from uint8array, then we are supporting those APIs. + +Domenic: I think it’s confusing. Either the TypedArray methods are supported or they aren’t — null them out if they’re not. + +Jeremiah: Probably nulling these things out is a good idea. + +James: in the docs we tell folks that it is a Uint8Array. + +Trevor: Documenting sounds great to me — I can get on that if everyone agrees. + +### doc: add Docs working group [#4244](https://github.com/nodejs/node/pull/4244) + +Chris: current state: call for new members (GitHub, Twitter), got significant reach, several people raising hands. Worked to define what membership means in the governance doc. Onboarding people that raised their hands. Had a second meeting for the group today. Everyone happy to work within the node repo, transition period with guides in the website repo before we get them building in the node repo. Don’t want to block core work so happy to do post-review of docs instead of holding up merges. Have a slack for collaboration now too. + +Rod: what would the post-review process look like? Similar to what we have now with the addition of review for style etc. from docs group? + +Chris: Yes, normal process, lgtm from collaborators, etc. Do a weekly review of the changes in the docs directory and do updates for that. Use slack for this by having a GitHub integration. + +Rod: Happy with progress? + +Chris: yes, going well. Building a Roadmap now for what’s going to be done. Need to telegraph what’s going to be done before it’s done. + +Jeremiah: nobody has worked on the doctool for years, is that on the roadmap? + +Chris: Plan is to build something new alongside it. + +Chris: What else needs to be done to move forward with ratification? + +Rod: Sounds like the boxes have been ticked so probably best to move back to the PR-to-core process and bring it for a vote at this meeting. + +### Seek legal advice on LICENSE and copyright blocks in code [#3979](https://github.com/nodejs/node/issues/3979) + +Rod: No progress on that. There’s supposed be another meeting, maybe next week — no material update. I’ll keep it on the agenda to make sure we’re updated with the progress. + +### fs: optimize realpath using uv_fs_realpath() [#3594](https://github.com/nodejs/node/pull/3594) + +Trevor: There’s now realpath in libuv, making the syscall is much faster than going through the JS logic we now have. Using the cache reduces the number of lstat calls — the question is: can we work towards changing the current realpath impl to call realpath in libuv more frequently as our investigation shows is useful — since it’s faster than doing the JS logic, even with the cache. + +Jeremiah: Were there concerns about changing behavior? + +Trevor: If you give it a bad cache it would no longer fail. Some of those issues were nonissues — there’s another one about symlinks. Little edge cases that we wouldn’t consider a problem. + +Jeremiah: If we use realpath(2) that would affect the module resolution? + +Bert: Module also goes through realpath. Hopefully it doesn’t change semantics if impl changes. If the semantics stay the same, it shouldn’t be an issue. Trevor, I’ll give you an issue on why it is the way it is. On OSX, realpath could trigger a buffer overflow, there was no workaround but to write your own. On windows, it used to not exist. We used readlink on all platforms to build our realpath. + +Trevor: To clarify: we make a libuv call, not a syscall + +Bert: Libuv would need to implement this logic — maybe Ben knows? + +Ben: The issue where you could not safely allocate a buffer for the call to store its result in? That used to be an issue with OSX until 10.7 I believe, but we don’t support that anymore, so should not be an issue anymore + +Bert: If we drop WinXP support, it becomes very easy to support + +Trevor: I think we have + +Bert: I’m pretty sure it does not — [cd: might need filled in] + +Trevor: Going to test more, if the results support what you said, are you OK with using the native implementation? This boiled down to throwing away the second optional argument of the cache. + +Bert: The cache has other issues — it can be inconsistent with what’s on disk, for example + +Trevor: We’ll test it and post results. Thank you. + +Bert: cool. + +Rod: do we need to open an issue to officially drop support WinXP? + +Alexis: Changing to prevent install? + +Bert: Sooner rather than later, I think. It’d have to be a major version, and libuv would have to bump to v2 before we could drop the platform. + +Jeremiah: I don’t think that’s an issue on our end. + +Bert: It is — + +Rod: Alexis, are you in on that discussion? + +Alexis: it was a constraint of node, I thought. Everybody agreed that we would prevent node startup on winxp. + +Rod: Back to GitHub! + +### ES Module EP [nodejs/node-eps#3](https://github.com/nodejs/node-eps/pull/3) + +Trevor: One quick thing: bmeck has been working on a EP for ES2015 module loading, figuring out how it could work in node alongside commonJS — originally it was going to be [a?]synchronous. He posted a proposed API for V8, they agreed to implement it, but they want assurances that the CTC agrees on that API. What I want to say: everybody go read the module spec and +1 it. To say “Yes, if V8 adds this API, we will use it to implement ES2015 modules and the Loader spec, too.” Any questions on that? + +## Next Meeting + +2016-01-27 From 6363264fa931fe79dc102c5456a906fdfb62b8f7 Mon Sep 17 00:00:00 2001 From: Alexander Makarenko Date: Thu, 4 Feb 2016 16:13:43 +0300 Subject: [PATCH 04/15] doc: fix links order in Buffer doc Sort links in lexical order PR-URL: https://github.com/nodejs/node/pull/5076 Reviewed-By: Sakthipriyan Vairamani Reviewed-By: Roman Klauke Reviewed-By: James M Snell --- doc/api/buffer.markdown | 17 ++++++++--------- 1 file changed, 8 insertions(+), 9 deletions(-) diff --git a/doc/api/buffer.markdown b/doc/api/buffer.markdown index 85e87a6b6fc8f8..4206d0778de596 100644 --- a/doc/api/buffer.markdown +++ b/doc/api/buffer.markdown @@ -75,8 +75,8 @@ The character encodings currently supported by Node.js include: Buffers are also `Uint8Array` TypedArray instances. However, there are subtle incompatibilities with the TypedArray specification in ECMAScript 2015. For instance, while `ArrayBuffer#slice()` creates a copy of the slice, -the implementation of [`Buffer#slice()`][] creates a view over the existing -Buffer without copying, making `Buffer#slice()` far more efficient. +the implementation of [`Buffer#slice()`][`buf.slice()`] creates a view over the +existing Buffer without copying, making `Buffer#slice()` far more efficient. It is also possible to create new TypedArray instances from a `Buffer` with the following caveats: @@ -1351,17 +1351,16 @@ socket.on('readable', () => { Use of `SlowBuffer` should be used only as a last resort *after* a developer has observed undue memory retention in their applications. -[iterator]: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Iteration_protocols -[`Array#indexOf()`]: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Array/indexOf [`Array#includes()`]: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Array/includes +[`Array#indexOf()`]: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Array/indexOf +[`buf.entries()`]: #buffer_buf_entries [`buf.fill(0)`]: #buffer_buf_fill_value_offset_end +[`buf.keys()`]: #buffer_buf_keys [`buf.slice()`]: #buffer_buf_slice_start_end +[`buf.values()`]: #buffer_buf_values [`buf1.compare(buf2)`]: #buffer_buf_compare_otherbuffer -[`Buffer#slice()`]: #buffer_buf_slice_start_end +[`JSON.stringify()`]: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/JSON/stringify [`RangeError`]: errors.html#errors_class_rangeerror [`String.prototype.length`]: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/String/length [`util.inspect()`]: util.html#util_util_inspect_object_options -[`buf.values()`]: #buffer_buf_values -[`buf.keys()`]: #buffer_buf_keys -[`buf.entries()`]: #buffer_buf_entries -[`JSON.stringify()`]: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/JSON/stringify +[iterator]: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Iteration_protocols From e05bb409a6c104d9ada035c42126d88a087c34a3 Mon Sep 17 00:00:00 2001 From: Rich Trott Date: Wed, 3 Feb 2016 12:27:40 -0800 Subject: [PATCH 05/15] tools: lint for spacing around unary operators MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Enable `space-unary-ops` in `.eslintrc`. This prohibits things like: i ++ // use `i++` instead typeof(foo) // use `typeof foo` or `typeof (foo)` instead Ref: https://github.com/nodejs/node/pull/4772#discussion_r51732299 PR-URL: https://github.com/nodejs/node/pull/5063 Reviewed-By: Colin Ihrig Reviewed-By: Michaël Zasso Reviewed-By: Roman Reiss Reviewed-By: James M Snell --- .eslintrc | 2 ++ lib/timers.js | 2 +- test/gc/test-http-client-connaborted.js | 2 +- test/gc/test-http-client-onerror.js | 2 +- test/gc/test-http-client-timeout.js | 2 +- test/gc/test-http-client.js | 2 +- test/gc/test-net-timeout.js | 2 +- test/parallel/test-child-process-exec-buffer.js | 4 ++-- test/parallel/test-fs-read-stream-inherit.js | 2 +- test/parallel/test-fs-read-stream.js | 2 +- test/parallel/test-http-byteswritten.js | 2 +- test/parallel/test-http-date-header.js | 2 +- test/parallel/test-http-response-multiheaders.js | 2 +- test/parallel/test-http-set-trailers.js | 2 +- test/parallel/test-https-byteswritten.js | 2 +- test/parallel/test-vm-debug-context.js | 4 ++-- test/sequential/test-cluster-listening-port.js | 2 +- 17 files changed, 20 insertions(+), 18 deletions(-) diff --git a/.eslintrc b/.eslintrc index e4dd84b8893da0..9f117dbacecb8a 100644 --- a/.eslintrc +++ b/.eslintrc @@ -81,6 +81,8 @@ rules: space-after-keywords: 2 ## no leading/trailing spaces in parens space-in-parens: [2, "never"] + ## no spaces with non-word unary operators, require for word unary operators + space-unary-ops: 2 # ECMAScript 6 # list: http://eslint.org/docs/rules/#ecmascript-6 diff --git a/lib/timers.js b/lib/timers.js index 543a1bf601c7ea..c15b58a17e9608 100644 --- a/lib/timers.js +++ b/lib/timers.js @@ -318,7 +318,7 @@ function unrefdHandle() { Timeout.prototype.unref = function() { if (this._handle) { this._handle.unref(); - } else if (typeof(this._onTimeout) === 'function') { + } else if (typeof this._onTimeout === 'function') { var now = Timer.now(); if (!this._idleStart) this._idleStart = now; var delay = this._idleStart + this._idleTimeout - now; diff --git a/test/gc/test-http-client-connaborted.js b/test/gc/test-http-client-connaborted.js index 5accec4ec63069..074457a7511fc5 100644 --- a/test/gc/test-http-client-connaborted.js +++ b/test/gc/test-http-client-connaborted.js @@ -48,7 +48,7 @@ for (var i = 0; i < 10; i++) getall(); function afterGC() { - countGC ++; + countGC++; } var timer; diff --git a/test/gc/test-http-client-onerror.js b/test/gc/test-http-client-onerror.js index e94bde6b5a20d1..2e5ef6a54288d4 100644 --- a/test/gc/test-http-client-onerror.js +++ b/test/gc/test-http-client-onerror.js @@ -56,7 +56,7 @@ function runTest() { } function afterGC() { - countGC ++; + countGC++; } var timer; diff --git a/test/gc/test-http-client-timeout.js b/test/gc/test-http-client-timeout.js index 1f87bce2336059..d80cf9cdee1532 100644 --- a/test/gc/test-http-client-timeout.js +++ b/test/gc/test-http-client-timeout.js @@ -57,7 +57,7 @@ for (var i = 0; i < 10; i++) getall(); function afterGC() { - countGC ++; + countGC++; } var timer; diff --git a/test/gc/test-http-client.js b/test/gc/test-http-client.js index 0cceb1edb015d7..73f284a042c37a 100644 --- a/test/gc/test-http-client.js +++ b/test/gc/test-http-client.js @@ -51,7 +51,7 @@ for (var i = 0; i < 10; i++) getall(); function afterGC() { - countGC ++; + countGC++; } setInterval(status, 1000).unref(); diff --git a/test/gc/test-net-timeout.js b/test/gc/test-net-timeout.js index 26e03786a68542..24671a647e0eac 100644 --- a/test/gc/test-net-timeout.js +++ b/test/gc/test-net-timeout.js @@ -57,7 +57,7 @@ for (var i = 0; i < 10; i++) getall(); function afterGC() { - countGC ++; + countGC++; } setInterval(status, 100).unref(); diff --git a/test/parallel/test-child-process-exec-buffer.js b/test/parallel/test-child-process-exec-buffer.js index a381af9631e0d8..6f19ac859a783f 100644 --- a/test/parallel/test-child-process-exec-buffer.js +++ b/test/parallel/test-child-process-exec-buffer.js @@ -10,8 +10,8 @@ var str = 'hello'; // default encoding exec('echo ' + str, function(err, stdout, stderr) { - assert.ok('string', typeof(stdout), 'Expected stdout to be a string'); - assert.ok('string', typeof(stderr), 'Expected stderr to be a string'); + assert.ok('string', typeof stdout, 'Expected stdout to be a string'); + assert.ok('string', typeof stderr, 'Expected stderr to be a string'); assert.equal(str + os.EOL, stdout); success_count++; diff --git a/test/parallel/test-fs-read-stream-inherit.js b/test/parallel/test-fs-read-stream-inherit.js index 2261b269f314e8..2b3a2f8f48584d 100644 --- a/test/parallel/test-fs-read-stream-inherit.js +++ b/test/parallel/test-fs-read-stream-inherit.js @@ -55,7 +55,7 @@ file.on('close', function() { var file3 = fs.createReadStream(fn, Object.create({encoding: 'utf8'})); file3.length = 0; file3.on('data', function(data) { - assert.equal('string', typeof(data)); + assert.equal('string', typeof data); file3.length += data.length; for (var i = 0; i < data.length; i++) { diff --git a/test/parallel/test-fs-read-stream.js b/test/parallel/test-fs-read-stream.js index e6aa997cc5b6a9..7cf0cd9edfc8fd 100644 --- a/test/parallel/test-fs-read-stream.js +++ b/test/parallel/test-fs-read-stream.js @@ -55,7 +55,7 @@ file.on('close', function() { var file3 = fs.createReadStream(fn, {encoding: 'utf8'}); file3.length = 0; file3.on('data', function(data) { - assert.equal('string', typeof(data)); + assert.equal('string', typeof data); file3.length += data.length; for (var i = 0; i < data.length; i++) { diff --git a/test/parallel/test-http-byteswritten.js b/test/parallel/test-http-byteswritten.js index fdc407aec667ba..41d8f4d40d1bf7 100644 --- a/test/parallel/test-http-byteswritten.js +++ b/test/parallel/test-http-byteswritten.js @@ -16,7 +16,7 @@ var httpServer = http.createServer(function(req, res) { res.on('finish', function() { sawFinish = true; - assert(typeof(req.connection.bytesWritten) === 'number'); + assert(typeof req.connection.bytesWritten === 'number'); assert(req.connection.bytesWritten > 0); }); res.writeHead(200, { 'Content-Type': 'text/plain' }); diff --git a/test/parallel/test-http-date-header.js b/test/parallel/test-http-date-header.js index 4c73800a8e908c..5ed7fc620bc475 100644 --- a/test/parallel/test-http-date-header.js +++ b/test/parallel/test-http-date-header.js @@ -6,7 +6,7 @@ var http = require('http'); var testResBody = 'other stuff!\n'; var server = http.createServer(function(req, res) { - assert.ok(! ('date' in req.headers), + assert.ok(!('date' in req.headers), 'Request headers contained a Date.'); res.writeHead(200, { 'Content-Type': 'text/plain' diff --git a/test/parallel/test-http-response-multiheaders.js b/test/parallel/test-http-response-multiheaders.js index 4705548d3a385f..83171bb475fd57 100644 --- a/test/parallel/test-http-response-multiheaders.js +++ b/test/parallel/test-http-response-multiheaders.js @@ -59,7 +59,7 @@ server.listen(common.PORT, common.mustCall(function() { http.get( {port:common.PORT, headers:{'x-num': n}}, common.mustCall(function(res) { - if (++ count === 2) server.close(); + if (++count === 2) server.close(); assert.equal(res.headers['content-length'], 1); for (const name of norepeat) { assert.equal(res.headers[name], 'A'); diff --git a/test/parallel/test-http-set-trailers.js b/test/parallel/test-http-set-trailers.js index f3ee5b157f7915..000df0189a9286 100644 --- a/test/parallel/test-http-set-trailers.js +++ b/test/parallel/test-http-set-trailers.js @@ -33,7 +33,7 @@ server.on('listening', function() { c.on('end', function() { c.end(); - assert.ok(! /x-foo/.test(res_buffer), 'Trailer in HTTP/1.0 response.'); + assert.ok(!/x-foo/.test(res_buffer), 'Trailer in HTTP/1.0 response.'); outstanding_reqs--; if (outstanding_reqs == 0) { server.close(); diff --git a/test/parallel/test-https-byteswritten.js b/test/parallel/test-https-byteswritten.js index beef89b25ccb98..4d42714f3dc4c0 100644 --- a/test/parallel/test-https-byteswritten.js +++ b/test/parallel/test-https-byteswritten.js @@ -18,7 +18,7 @@ var body = 'hello world\n'; var httpsServer = https.createServer(options, function(req, res) { res.on('finish', function() { - assert(typeof(req.connection.bytesWritten) === 'number'); + assert(typeof req.connection.bytesWritten === 'number'); assert(req.connection.bytesWritten > 0); httpsServer.close(); console.log('ok'); diff --git a/test/parallel/test-vm-debug-context.js b/test/parallel/test-vm-debug-context.js index 7789ddc83fc18e..07335fad56a1f7 100644 --- a/test/parallel/test-vm-debug-context.js +++ b/test/parallel/test-vm-debug-context.js @@ -21,8 +21,8 @@ assert.throws(function() { vm.runInDebugContext('(function(f) { f(f) })(function(f) { f(f) })'); }, /RangeError/); -assert.equal(typeof(vm.runInDebugContext('this')), 'object'); -assert.equal(typeof(vm.runInDebugContext('Debug')), 'object'); +assert.equal(typeof vm.runInDebugContext('this'), 'object'); +assert.equal(typeof vm.runInDebugContext('Debug'), 'object'); assert.strictEqual(vm.runInDebugContext(), undefined); assert.strictEqual(vm.runInDebugContext(0), 0); diff --git a/test/sequential/test-cluster-listening-port.js b/test/sequential/test-cluster-listening-port.js index aaad1ff620b191..dc048c891c7b8c 100644 --- a/test/sequential/test-cluster-listening-port.js +++ b/test/sequential/test-cluster-listening-port.js @@ -12,7 +12,7 @@ if (cluster.isMaster) { // ensure that the port is not 0 or null assert(port); // ensure that the port is numerical - assert.strictEqual(typeof(port), 'number'); + assert.strictEqual(typeof port, 'number'); worker.kill(); }); process.on('exit', function() { From d64e6e0112839cd55e0ba49af83412b46d4b9988 Mon Sep 17 00:00:00 2001 From: HUANG Wei Date: Fri, 29 Jan 2016 18:15:32 +0800 Subject: [PATCH 06/15] buffer: remove duplicated code in fromObject Add fromArrayLike() to handle logic of copying in values from array-like argument. PR-URL: https://github.com/nodejs/node/pull/4948 Reviewed-By: Sakthipriyan Vairamani Reviewed-By: James M Snell Reviewed-By: Trevor Norris --- lib/buffer.js | 35 ++++++++++++----------------------- test/parallel/test-buffer.js | 7 +++++++ 2 files changed, 19 insertions(+), 23 deletions(-) diff --git a/lib/buffer.js b/lib/buffer.js index bd2c859c3ee389..6b1621eae11081 100644 --- a/lib/buffer.js +++ b/lib/buffer.js @@ -117,6 +117,13 @@ function fromString(string, encoding) { return b; } +function fromArrayLike(obj) { + const length = obj.length; + const b = allocate(length); + for (let i = 0; i < length; i++) + b[i] = obj[i] & 255; + return b; +} function fromObject(obj) { if (obj instanceof Buffer) { @@ -129,14 +136,6 @@ function fromObject(obj) { return b; } - if (Array.isArray(obj)) { - const length = obj.length; - const b = allocate(length); - for (let i = 0; i < length; i++) - b[i] = obj[i] & 255; - return b; - } - if (obj == null) { throw new TypeError('must start with number, buffer, array or string'); } @@ -145,25 +144,15 @@ function fromObject(obj) { return binding.createFromArrayBuffer(obj); } - if (obj.buffer instanceof ArrayBuffer || obj.length) { - let length; - if (typeof obj.length !== 'number' || obj.length !== obj.length) - length = 0; - else - length = obj.length; - const b = allocate(length); - for (let i = 0; i < length; i++) { - b[i] = obj[i] & 255; + if (obj.buffer instanceof ArrayBuffer || 'length' in obj) { + if (typeof obj.length !== 'number' || obj.length !== obj.length) { + return allocate(0); } - return b; + return fromArrayLike(obj); } if (obj.type === 'Buffer' && Array.isArray(obj.data)) { - var array = obj.data; - const b = allocate(array.length); - for (let i = 0; i < array.length; i++) - b[i] = array[i] & 255; - return b; + return fromArrayLike(obj.data); } throw new TypeError('must start with number, buffer, array or string'); diff --git a/test/parallel/test-buffer.js b/test/parallel/test-buffer.js index 856518943b2049..8d13af3ea1baf7 100644 --- a/test/parallel/test-buffer.js +++ b/test/parallel/test-buffer.js @@ -28,6 +28,13 @@ var c = new Buffer(512); console.log('c.length == %d', c.length); assert.strictEqual(512, c.length); +var d = new Buffer([]); +assert.strictEqual(0, d.length); + +var ui32 = new Uint32Array(4).fill(42); +var e = Buffer(ui32); +assert.deepEqual(ui32, e); + // First check Buffer#fill() works as expected. assert.throws(function() { From f8269fe365a77c6c3c3e3750d9f8cae9065154b5 Mon Sep 17 00:00:00 2001 From: Myles Borins Date: Wed, 3 Feb 2016 16:07:44 -0800 Subject: [PATCH 07/15] querystring: check that maxKeys is finite There was a very subtle change in behavior introduced with 27def4f In the past if querystring.parse was given Infinity for maxKeys, everything worked as expected. Check to see is maxKeys is Infinity before forwarding the value to String.prototype.split which causes this regression PR-URL: https://github.com/nodejs/node/pull/5066 Reviewed-By: Evan Lucas Reviewed By: Sakthipriyan Vairamani Reviewed-By: Rod Vagg Reviewed-By: Jeremiah Senkpiel --- lib/querystring.js | 2 +- .../test-querystring-maxKeys-non-finite.js | 55 +++++++++++++++++++ 2 files changed, 56 insertions(+), 1 deletion(-) create mode 100644 test/parallel/test-querystring-maxKeys-non-finite.js diff --git a/lib/querystring.js b/lib/querystring.js index d5d4f175b6bebf..4244d8c18b8122 100644 --- a/lib/querystring.js +++ b/lib/querystring.js @@ -215,7 +215,7 @@ QueryString.parse = QueryString.decode = function(qs, sep, eq, options) { } // maxKeys <= 0 means that we should not limit keys count - if (maxKeys > 0) { + if (maxKeys > 0 && isFinite(maxKeys)) { qs = qs.split(sep, maxKeys); } else { qs = qs.split(sep); diff --git a/test/parallel/test-querystring-maxKeys-non-finite.js b/test/parallel/test-querystring-maxKeys-non-finite.js new file mode 100644 index 00000000000000..aab7c45f1b6bed --- /dev/null +++ b/test/parallel/test-querystring-maxKeys-non-finite.js @@ -0,0 +1,55 @@ +'use strict'; +// This test was originally written to test a regression +// that was introduced by +// https://github.com/nodejs/node/pull/2288#issuecomment-179543894 +require('../common'); + +const assert = require('assert'); +const parse = require('querystring').parse; + +/* +taken from express-js/body-parser +https://github.com/expressjs/body-parser/ +blob/ed25264fb494cf0c8bc992b8257092cd4f694d5e/test/urlencoded.js#L636-L651 +*/ +function createManyParams(count) { + var str = ''; + + if (count === 0) { + return str; + } + + str += '0=0'; + + for (var i = 1; i < count; i++) { + var n = i.toString(36); + str += '&' + n + '=' + n; + } + + return str; +} + +const count = 10000; +const originalMaxLength = 1000; +const params = createManyParams(count); + +// thealphanerd +// 27def4f introduced a change to parse that would cause Inifity +// to be passed to String.prototype.split as an argument for limit +// In this instance split will always return an empty array +// this test confirms that the output of parse is the expected length +// when passed Infinity as the argument for maxKeys +const resultInfinity = parse(params, undefined, undefined, {maxKeys: Infinity}); +const resultNaN = parse(params, undefined, undefined, {maxKeys: NaN}); +const resultInfinityString = parse(params, undefined, undefined, { + maxKeys: 'Infinity' +}); +const resultNaNString = parse(params, undefined, undefined, {maxKeys: 'NaN'}); + +// Non Finite maxKeys should return the length of input +assert.equal(Object.keys(resultInfinity).length, count); +assert.equal(Object.keys(resultNaN).length, count); +// Strings maxKeys should return the maxLength +// defined by parses internals +assert.equal(Object.keys(resultInfinityString).length, originalMaxLength); +assert.equal(Object.keys(resultNaNString).length, originalMaxLength); From 1b49dfbe78edfc0fe568b39d5f05b4d60b18481f Mon Sep 17 00:00:00 2001 From: Myles Borins Date: Mon, 1 Feb 2016 11:13:31 -0800 Subject: [PATCH 08/15] node_contextify: do not incept debug context Currently a debug context is created for various calls to util. If the node debugger is being run the main context is the debug context. In this case node_contextify was freeing the debug context and causing everything to explode. This change moves around the logic and no longer frees the context. There is a concern about the dangling pointer The regression test was adapted from code submitted by @3y3 in #4815 Fixes: https://github.com/nodejs/node/issues/4440 Fixes: https://github.com/nodejs/node/issues/4815 Fixes: https://github.com/nodejs/node/issues/4597 Fixes: https://github.com/nodejs/node/issues/4952 PR-URL: https://github.com/nodejs/node/issues/4815 Reviewed-By: Fedor Indutny Reviewed-By: Ben Noordhuis Reviewed-By: Rich Trott --- src/node_contextify.cc | 31 +++------ .../debugger-util-regression-fixture.js | 4 ++ .../parallel/test-debugger-util-regression.js | 69 +++++++++++++++++++ 3 files changed, 83 insertions(+), 21 deletions(-) create mode 100644 test/fixtures/debugger-util-regression-fixture.js create mode 100644 test/parallel/test-debugger-util-regression.js diff --git a/src/node_contextify.cc b/src/node_contextify.cc index 6675e2d8a08f70..dc6695893cc609 100644 --- a/src/node_contextify.cc +++ b/src/node_contextify.cc @@ -239,37 +239,26 @@ class ContextifyContext { static void RunInDebugContext(const FunctionCallbackInfo& args) { - // Ensure that the debug context has an Environment assigned in case - // a fatal error is raised. The fatal exception handler in node.cc - // is not equipped to deal with contexts that don't have one and - // can't easily be taught that due to a deficiency in the V8 API: - // there is no way for the embedder to tell if the data index is - // in use. - struct ScopedEnvironment { - ScopedEnvironment(Local context, Environment* env) - : context_(context) { - const int index = Environment::kContextEmbedderDataIndex; - context->SetAlignedPointerInEmbedderData(index, env); - } - ~ScopedEnvironment() { - const int index = Environment::kContextEmbedderDataIndex; - context_->SetAlignedPointerInEmbedderData(index, nullptr); - } - Local context_; - }; - Local script_source(args[0]->ToString(args.GetIsolate())); if (script_source.IsEmpty()) return; // Exception pending. Local debug_context = Debug::GetDebugContext(); + Environment* env = Environment::GetCurrent(args); if (debug_context.IsEmpty()) { // Force-load the debug context. Debug::GetMirror(args.GetIsolate()->GetCurrentContext(), args[0]); debug_context = Debug::GetDebugContext(); CHECK(!debug_context.IsEmpty()); + // Ensure that the debug context has an Environment assigned in case + // a fatal error is raised. The fatal exception handler in node.cc + // is not equipped to deal with contexts that don't have one and + // can't easily be taught that due to a deficiency in the V8 API: + // there is no way for the embedder to tell if the data index is + // in use. + const int index = Environment::kContextEmbedderDataIndex; + debug_context->SetAlignedPointerInEmbedderData(index, env); } - Environment* env = Environment::GetCurrent(args); - ScopedEnvironment env_scope(debug_context, env); + Context::Scope context_scope(debug_context); Local