Update node-chakracore with nodejs/master changes #25
Conversation
Refs: nodejs/node#3254 PR-URL: nodejs/node#4540 Reviewed-By: James M Snell <jasnell@gmail.com>
`clientError` will have `http.Server`-specific behavior, and we don't want to shadow it in `tls.Server`. PR-URL: nodejs/node#4557 Reviewed-By: Brian White <mscdex@mscdex.net>
Make default `clientError` behavior (close socket immediately)
overridable. With this APIs it is possible to write a custom error
handler, and to send, for example, a 400 HTTP response.
http.createServer(...).on('clientError', function(err, socket) {
socket.end('HTTP/1.1 400 Bad Request\r\n\r\n');
socket.destroy();
});
Fix: #4543
PR-URL: nodejs/node#4557
Reviewed-By: Brian White <mscdex@mscdex.net>
Adds Myles Borins and his public key to the README PR-URL: nodejs/node#4578 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Evan Lucas <evanlucas@me.com>
Adds Evan Lucas and his public key to the README for releases PR-URL: nodejs/node#4579 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Myles Borins <myles.borins@gmail.com>
It avoids the creation of unnecessary handles. This issue is causing intermitent failures in `test-cluster-disconnect-race` on `FreeBSD` and `OS X`. The problem is that the `worker2.disconnect` is being called on the master before the `queryServer` is handled, causing the worker to be deleted, then the Server handle is created afterwards. Later on, when `removeWorker` is called from the `exit` handler, there are no workers left, but one handle, thus the `AssertionError`. Add a new `test/sequential/test-cluster-disconnect-leak` based on `test-cluster-disconnect-race` that creates lots of workers and fails consistently without this patch. PR-URL: nodejs/node#4465 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Johan Bergström <bugs@bergstroem.nu> Reviewed-By: Rich Trott <rtrott@gmail.com>
fix description about the latest LTS release download page to make it clear PR-URL: nodejs/node#4583 Reviewed-By: Rod Vagg <rod@vagg.org> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>
`V4MAPPED` isn't supported by Android either (as of 6.0) PR-URL: nodejs/node#4580 Reviewed-By: Brian White <mscdex@mscdex.net> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Missed on the previous review, minor line wrapping nit PR-URL: nodejs/node#4588 Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
The server method returns `self` in order to allow chaining. PR-URL: nodejs/node#4590 Fixes: nodejs/node#4571 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Roman Reiss <me@silverwind.io>
`Server`, `ServerResponse` etc. were marked as classes, this one class was overlooked. PR-URL: nodejs/node#4589 Fixes: nodejs/node#4576 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Roman Reiss <me@silverwind.io>
These have been deprecated since Apr 27, 2013, and the plan was to remove them in "node v0.13". buffer.get(index) is superseded by buffer[index]. buffer.set(index, value) is superseded by buffer[index] = value. These have never been documented at any point in node's history. PR-URL: nodejs/node#4594 Fixes: nodejs/node#4587 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Trevor Norris <trev.norris@gmail.com> Reviewed-By: Brian White <mscdex@mscdex.net> Reviewed-By: Roman Reiss <me@silverwind.io>
Fixes: nodejs/node#4532 PR-URL: nodejs/node#4535 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Evan Lucas <evanlucas@me.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Sam Roberts <vieuxtech@gmail.com> Reviewed-By: Roman Reiss <me@silverwind.io>
Remove unnecessary `setImmediate()` that causes a minor race condition. Stop the test after 3 occurrences rather than 5 to allow for slower hosts running the test in parallel with other tests. Fixes: nodejs/node#4559 PR-URL: nodejs/node#4599 Reviewed-By: Johan Bergström <bugs@bergstroem.nu>
* http: - A new status code was added: 451 - "Unavailable For Legal Reasons" (Max Barinov) nodejs/node#4377 - Idle sockets that have been kept alive now handle errors (José F. Romaniello) nodejs/node#4482 * This release also includes several minor performance improvements: - assert: deepEqual is now speedier when comparing TypedArrays (Claudio Rodriguez) nodejs/node#4330 - lib: Use arrow functions instead of bind where possible (Minwoo Jung) nodejs/node#3622 - node: Improved accessor perf of process.env (Trevor Norris) nodejs/node#3780 - node: Improved performance of process.hrtime() (Trevor Norris) nodejs/node#3780, (Evan Lucas) nodejs/node#4484 - node: Improved GetActiveHandles performance (Trevor Norris) nodejs/node#3780 - util: Use faster iteration in util.format() (Jackson Tian) nodejs/node#3964 Refs: nodejs/node#4547 PR-URL: nodejs/node#4623 Reviewed-By: Roman Reiss <me@silverwind.io> Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
PR-URL: nodejs/node#4617 Reviewed-By: Roman Reiss <me@silverwind.io> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Brian White <mscdex@mscdex.net>
- Changed colors to match frontpage as close as possible. - Links are slightly more horizontally padded as compared before to accomodate for the hover effect. - Slightly reduced the scroll indication height on the TOC. - The main content is now offset using margin instead of the previous border hack. - remove empty footer that was rendering a dark bar on the bottom of each page without any content. PR-URL: nodejs/node#4621 Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Brian White <mscdex@mscdex.net>
Previously, Error objects were formatted as the result of a `toString` call bounded by square brackets. They are now formatted as the stack trace for the given error object. The intention initially was to emulate how browsers do `console.error` but since that would also impact `console.warn`, `console.log`, etc, it was decided to make the change at `util.inspect` level which is in turn used by the `console` package. Fixes: nodejs#4452 PR-URL: nodejs/node#4582 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Trevor Norris <trev.norris@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>
The docs were recently refactored, and some "above" and "below" references were no longer accurate. This commit removes many such references, and replaces others with links. PR-URL: nodejs/node#4499 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>
A few tests in test/gc include the http module twice. Remove duplicate require(). PR-URL: nodejs/node#4606 Reviewed-By: Brian White <mscdex@mscdex.net> Reviewed-By: Michaël Zasso <mic.besace@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>
The exts and trailingSlash variables are only used if the path isn't cached. This commit moves them further down in the code, and changes from var to const. PR-URL: nodejs/node#3579 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Trevor Norris <trev.norris@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Michaël Zasso <mic.besace@gmail.com>
Two tests were requiring the common module twice. This removes the duplicate require statement in the tests. PR-URL: nodejs/node#4611 Reviewed-By: Brian White <mscdex@mscdex.net> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Johan Bergström <bugs@bergstroem.nu> Reviewed-By: James M Snell <jasnell@gmail.com>
Remove redeclarations of variables in node.js. This includes removing
one apparently unnecessary `NativeModule.require('module')`.
PR-URL: nodejs/node#4605
Reviewed-By: Brian White <mscdex@mscdex.net>
Reviewed-By: Michaël Zasso <mic.besace@gmail.com>
Reviewed-By: Johan Bergström <bugs@bergstroem.nu>
Reviewed-By: James M Snell <jasnell@gmail.com>
In lib/_http_client.js, the variable `conn` was declared with the `var` keyword three times in the same scope. This change eliminates the variable entirely. PR-URL: nodejs/node#4612 Reviewed-By: Michaël Zasso <mic.besace@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>
In order to make developers aware of node-core built-in functionality, which might replace module APIs, we should add an example of readline`s interface usage. SEO will eventually aid this goal, since it is well searched on Q&A sites. PR-URL: nodejs/node#4609 Reviewed-By: Roman Reiss <me@silverwind.io> Reviewed-By: James M Snell <jasnell@gmail.com>>
Add support to fs.createWriteStream and fs.createWriteStream for an autoClose option that behaves similarly to the autoClose option supported by fs.createReadStream and fs.ReadStream. When an instance of fs.createWriteStream created with autoClose === false finishes, it is not destroyed. Its underlying fd is not closed and it is the responsibility of the user to close it. PR-URL: nodejs/node#3679 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Do not attempt to read data from the socket whilst on OpenSSL's stack, weird things may happen, and this is most likely going to result in some kind of error. PR-URL: nodejs/node#4624 Reviewed-By: Trevor Norris <trev.norris@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>
`fork` is imported twice in a row. Remove duplication. PR-URL: nodejs/node#4634 Reviewed-By: Johan Bergström <bugs@bergstroem.nu> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Brian White <mscdex@mscdex.net> Reviewed-By: Michaël Zasso <mic.besace@gmail.com>
This comment was added with an assumption that we could determine the IP address that localhost should resolve to without performing a lookup. This was a false assumption and should be removed. PR-URL: nodejs/node#4648 Reviewed-By: Brian White <mscdex@mscdex.net> Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
It's not guaranteed that the socket data is received in the same chunk as the upgrade response. Listen for the `data` event to make sure all the data is received. PR-URL: nodejs/node#4520 Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>
|
@orangemocha Not completely sure, but it looks like this commit nodejs/node@4e4b260 fixing the |
|
test-process-getactivehandles is flaky on nodejs/node:master :( https://ci.nodejs.org/job/node-stress-single-test/504/nodes=pi2-raspbian-wheezy/console |
|
@santigimeno you are right, this PR is only including changes from master up to 7406cd3, so the fix for those flaky tests are missing. I'll follow up on test-process-getactivehandles in nodejs/node. LGTM |
|
Thanks @orangemocha , @thefourtheye for review. |
* Implemented `Private` that is used as private symbol to store on object. * GetPrivate * SetPrivate * HasPrivate * DeletePrivate * Added `CachedData` field on `ScriptCompiler::Source` that chakra don't support. So for now, it returns nullptr that crashes one of the unit test. * Added 'HeapSpaceStatistics' API from v8 into chakrashim. * Unit test bug fixes PR-URL: nodejs#25 Reviewed-By: Alexis Campailla <orangemocha@nodejs.org> Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com> Reviewed-By: Jianchun Xu <Jianchun.Xu@microsoft.com>
* Disabled several debugger test because chakracore doesn't have support yet. * Skip `Buffer.toString()` test that verifies using v8 specific `kStringMaxLength` variable. * Because of error difference, added baselines for some test for chakra This gets us to 29 failures which are either not implemented or bugs in chakrashim. PR-URL: nodejs#25 Reviewed-By: Alexis Campailla <orangemocha@nodejs.org> Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com> Reviewed-By: Jianchun Xu <Jianchun.Xu@microsoft.com>
Includes below bug fixes in chakracore: * Fix to chakracore that enables chakracore work on windows 7 without IE11. * field copy-prop bug fix. See Microsoft/Chakracore [release/1.1](https://github.com/Microsoft/ChakraCore/commits/release/1.1) for more details. PR-URL: nodejs#25 Reviewed-By: Jianchun Xu <Jianchun.Xu@microsoft.com>
kunalspathak
force-pushed
the
chakracore-next
branch
from
397d182 to
4cf733e
Compare
|
Updated the commits with metadata information. |
|
Merge commits are not preferred. Can you get rid off them? |
@thefourtheye Not sure if there's an easy way to do that. It would involve cherry picking each commit from nodejs/node:master, which would be laborious and error-prone. We did use merge commits in Core for a long time, until recently we changed to cherry-picking commits from master to the LTS branches. It's easier there because LTS branches hardly ever have changes of their own. |
|
Agree with @orangemocha . With rebase we will lose the original commit hashes and we won't be able to follow branches in which commits propagated. |
|
That's true. Hmmm, how about this? Going forward, we can probably have syncing(merging) with node/master as a separate PR, which can have merge commits. But other PRs can be landed manually like we do in nodejs core (with the similar commit log modifications, like PR-URL and Reviewed-By). Will that be okay? |
|
Yep. And the net effect of this PR is what you describe (I think). There is one merge commit and then a few node-chakracore specific changes, including deps updates, and the commits that are not from master have the standard commit metadata. The engine updates might however be necessary with the same PR as the integration from master, or otherwise chakracore-master might be broken. |
The problem with that is there could be v8 API changes that needs equivalent chakrashim changes to build node+chakracore. If we don't submit a single commit for that (merge + chakrashim) changes then a head that just has nodejs merge will be useless because it won't build. This PR address that by having node changes merged followed by chakrashim, core changes in 2 separate commits. |
Makes perfect sense. Thanks @orangemocha & @kunalspathak for helping me understand :-) |
|
@kunalspathak About the commit metadata, we normally update them when landing the changes and the Reviewed-By field has the collaborators who explicitly say that the patch looks okay in the PR. In the commits, I see "Jianchun Xu" and I don't see their approval in the PR. Are we missing something? |
|
@thefourtheye , thanks for pointing it out, will remember to update when landing the changes instead of still in PR. "Jianchun Xu" reviewed chakrashim changes in private repo. Should I get sign off from me on this PR or its ok not to include him as a reviewer? |
|
@kunalspathak It would be better if we can have explicit sign-offs in all our PRs. |
|
@thefourtheye , Sure. @jianchun , can you please sign off. |
I double checked and yes the "Update chakracore to 1.1.0.3" commit is good. |
And the other commits LGTM too... I reviewed before. (Sorry clicked too fast). |
|
FYI, I unpacked this PR and noticed an issue, filed nodejs/node@76cb81b#commitcomment-16209618 |
|
I have already opened an issue at nodejs/node#5094. Sent from Outlook Mobilehttps://aka.ms/blhgte On Fri, Feb 19, 2016 at 10:19 AM -0800, "Jianchun Xu" <notifications@github.commailto:notifications@github.com> wrote: FYI, I unpacked this PR and noticed an issue, filed nodejs/node@76cb81b#commitcomment-16209618nodejs/node@76cb81b#commitcomment-16209618 Reply to this email directly or view it on GitHubhttps://github.com//pull/25#issuecomment-186345050. |
Great! |
Includes below bug fixes in chakracore: * Fix to chakracore that enables chakracore work on windows 7 without IE11. * field copy-prop bug fix. See Microsoft/Chakracore [release/1.1](https://github.com/Microsoft/ChakraCore/commits/release/1.1) for more details. PR-URL: #25 Reviewed-By: Jianchun Xu <Jianchun.Xu@microsoft.com> Fixes: #20
This PR updates node-chakracore with 7406cd3 of nodejs/node "master" branch. All the commits, except last 3 commits are from nodejs/node repo and are present to retain the history.
Summary of last 3 commits are as follows: