-
Notifications
You must be signed in to change notification settings - Fork 29.8k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
iojs.exe randomly terminates on Windows #1512
Comments
@alexlamsl can you give us any other info? io.js and windows versions? Memory usage? |
Sorry @Fishrock123 - mistyped |
When the
I have encountered this issue on io.js 1.7.1 and 1.8.1 - I have not tried any earlier versions. |
@alexlamsl does it happen if you have any two iojs processes running and quite one, or only when one is running http? |
In addition, as I first suspected this issue might have something to do with I tried running the tester process on a separate PC over the network, and cannot reproduce this problem so far. |
Under normal circumstances, the I suspect there might be occasions when those schedule processes terminate in an erroneous condition, and triggers this random termination behaviour on the |
Running just |
Something else that is probably irrelevant as well - when I listen to
|
Memory usage is around 60~250MB, on a machine that has 64GB, running Windows Server 2012 R2 |
If I were to run 3 tester processes simultaneously on the same machine as the However, the other 2 tester processes would continue to run, albeit not being able to connect to anything obviously, as the |
I just tried forceful termination of |
@Fishrock123 any other combination of tests you would like me to try? I will try and see if I can narrow something down over the weekend - but without any console output, I find a bit hard to even give an educated guess atm. |
Just a wild guess, but could you remove the I once had a similar issue on Linux with a |
@silverwind on Windows, Anyway, I tried removing
Problem still persists (in that the first process sometimes terminates, and when it does it may print an assertion error).
|
So far I can only get this to happen using |
I'm not sure you can discern a hardlink from a copy in the GUI, but it should be one. Anyways, it's not the cause of this issue at least, and I think a reduced test case is needed. |
@silverwind @Fishrock123 This is my best attempt thus far: server.js
client.js
Steps:
Expected:
Actual (sometimes, not always):
|
On the chances of hitting this bug, bigger files seem to help:
Sleeping does not help:
|
Was able to reproduce, test repo here: https://github.com/silverwind/iojs-issue-1512.git. With the repo cloned, run It looks to be Windows only, couldn't get it to reproduce on Linux. Also couldn't reproduce with
|
The assertion points to a file added in #667, but it might've been in place before. Might be worth to try v1.2.0. |
@silverwind Using the test case I've got (which I checked and believe to be identical to yours): Works fine:
Crashes:
|
That would mean it's somewhere in here: v1.5.1...v1.6.4 @alexlamsl can you narrow it down more between 1.5.1 and 1.6.4? |
@Fishrock123 tested on a different machine (Win10 Pro x64, 4GB RAM) Working:
Crashes:
When the |
I'll |
@silverwind not sure how much this helps, but I have an updated test case which decouples server.js
|
You could also create a big dataset through |
Yeah, doing that works, or just this even:
|
Dispatch requests in the implementation of the stream, not in the code creating these requests. The requests might be piled up and invoked internally in the implementation, so it should know better when it is the time to dispatch them. In fact, TLS was doing exactly this thing which led us to... Fix: #1512 PR-URL: #1563 Reviewed-By: Shigeki Ohtsu <[email protected]>
Should be temporarily fixed in 30b7349, and properly in a future libuv patch. Re-open if still an issue. |
PR-URL: #1532 Notable Changes: * crypto: significantly reduced memory usage for TLS (Fedor Indutny & Сковорода Никита Андреевич) #1529 * net: socket.connect() now accepts a 'lookup' option for a custom DNS resolution mechanism, defaults to dns.lookup() (Evan Lucas) #1505 * npm: Upgrade npm to 2.9.0. See the v2.8.4 and v2.9.0 release notes for details. Notable items: - Add support for default author field to make npm init -y work without user-input (@othiym23) npm/npm/d8eee6cf9d - Include local modules in npm outdated and npm update (@ArnaudRinquin) npm/npm#7426 - The prefix used before the version number on npm version is now configurable via tag-version-prefix (@kkragenbrink) npm/npm#8014 * os: os.tmpdir() is now cross-platform consistent and will no longer returns a path with a trailling slash on any platform (Christian Tellnes) #747 * process: - process.nextTick() performance has been improved by between 2-42% across the benchmark suite, notable because this is heavily used across core (Brian White) #1548 - New process.geteuid(), process.seteuid(id), process.getegid() and process.setegid(id) methods allow you to get and set effective UID and GID of the process (Evan Lucas) #1536 * repl: - REPL history can be persisted across sessions if the NODE_REPL_HISTORY_FILE environment variable is set to a user accessible file, NODE_REPL_HISTORY_SIZE can set the maximum history size and defaults to 1000 (Chris Dickinson) #1513 - The REPL can be placed in to one of three modes using the NODE_REPL_MODE environment variable: sloppy, strict or magic (default); the new magic mode will automatically run "strict mode only" statements in strict mode (Chris Dickinson) #1513 * smalloc: the 'smalloc' module has been deprecated due to changes coming in V8 4.4 that will render it unusable * util: add Promise, Map and Set inspection support (Christopher Monsanto) #1471 * V8: upgrade to 4.2.77.18, see the ChangeLog for full details. Notable items: - Classes have moved out of staging; the class keyword is now usable in strict mode without flags - Object literal enhancements have moved out of staging; shorthand method and property syntax is now usable ({ method() { }, property }) - Rest parameters (function(...args) {}) are implemented in staging behind the --harmony-rest-parameters flag - Computed property names ({['foo'+'bar']:'bam'}) are implemented in staging behind the --harmony-computed-property-names flag - Unicode escapes ('\u{xxxx}') are implemented in staging behind the --harmony_unicode flag and the --harmony_unicode_regexps flag for use in regular expressions * Windows: - Random process termination on Windows fixed (Fedor Indutny) #1512 / #1563 - The delay-load hook introduced to fix issues with process naming (iojs.exe / node.exe) has been made opt-out for native add-ons. Native add-ons should include 'win_delay_load_hook': 'false' in their binding.gyp to disable this feature if they experience problems . (Bert Belder) #1433 * Governance: - Rod Vagg (@rvagg) was added to the Technical Committee (TC) - Jeremiah Senkpiel (@Fishrock123) was added to the Technical Committee (TC)
@Fishrock123 I have just tried |
Reproduced >10 times without any errors in the console - |
Yes, the access violation is still there. |
May I ask you to give a try to indutny@df28a73 commit? Does applying it fix it? |
@indutny I tried https://gist.github.com/saghul/e05b99b1b16a7c9c15e9 but got some errors appying the patch
so I changed the patch like this (Really not knowing what I am doing there but I wanted it to compile to test if it works if libuv does not return the error synchronously)
And I tested a while and it seems to work. Sure, I can test your commit. |
@indutny the patchset is in C++, and I don't have the compiler infrastructure ready over here to test it. Sorry. |
@indutny I cherrypicked your commit, rebuilt and the server is still crashing but due to an other error |
@mathiask88 you were using the patch for latest libuv master, https://gist.github.com/saghul/52677dba652ab8e78051 is for the version io.js uses (#1563). |
Will try figure it out today on io.js level. I think that df28a73 should work after some tweaking. |
@silverwind Ah, ok thanks. Then I did everything right ;) However with the libuv patch this problem seems to be gone. |
I think @saghul intends to include the original fix for libuv/libuv#339 in libuv 1.5.0 (#1563 (comment)), maybe it' best to wait on that before applying more 'hacks'. 😉 |
And it landed: libuv/libuv@f880023. |
Should be fixed by 04cc03b. Closing, holler if I should reopen it. |
I verified my test case again, seems to be fixed for real this time. |
Verified on |
Dispatch requests in the implementation of the stream, not in the code creating these requests. The requests might be piled up and invoked internally in the implementation, so it should know better when it is the time to dispatch them. In fact, TLS was doing exactly this thing which led us to... Fix: nodejs#1512 PR-URL: nodejs#1563 Reviewed-By: Shigeki Ohtsu <[email protected]>
Fixes: nodejs#1397 Fixes: nodejs#1512 Fixes: nodejs#1621 Fixes: nodejs#862 PR-URL: nodejs#1646 Reviewed-By: Ben Noordhuis <[email protected]>
@silverwind Nah, you didn't.
Node 5.0.0 |
@polkovnikov-ph I believe you are hitting a different bug - may be file a new issue to track it? |
The
iojs.exe
process in question is running one ofhttp
andhttps
servers.The problem seems to occur if a separate
iojs
process is running, and is subsequently terminated, e.g. manually by pressingCtrl+C
within a Command Prompt window, then the aforementioned process would terminate as well, whether it is running in the background or in (a separate) Command Prompt window.The text was updated successfully, but these errors were encountered: