mirror of https://github.com/lukechilds/node.git
Browse Source
PR-URL: https://github.com/nodejs/node/pull/4904 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Roman Reiss <me@silverwind.io>process-exit-stdio-flushing
Rod Vagg
9 years ago
committed by
Roman Reiss
Roman Reiss
1 changed files with 337 additions and 0 deletions
@ -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**: <https://docs.google.com/document/d/1X3RbJhhLvFNojmQwxmsfUb2qPMyNPcVzcSS2aAoAon0> |
|||
* _Previous Minutes Google Doc: <https://docs.google.com/document/d/1084kuafyFax-1dymqvMlVUPvu3apdzw0ZM_4FX3z1RM>_ |
|||
|
|||
## 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 |
|||
Loading…
Reference in new issue