New Failures in AIX nightly run (possibly v8_inspector change) #7080
Comments
|
Here is the diff between build 194 (0 failures) and 195 (3 failures) |
|
re-run to see if they are consistent https://ci.nodejs.org/job/node-test-commit-aix/ |
|
Seem to fail consistently as failed in re-run |
|
Seems to be related to malloc(0) behaviour. By default AIX fails malloc(0) and we've told it to have the linux malloc(0) behaviour. Not sure how but it looks like this change has somehow affected that. Believe the failure in test-crypto-random is due to case where it does crypto.randomBytes(0) as this seems to fail with an out of memory. @ofrobots does the inspector do anything to hook or intercept malloc ? |
|
simplest test case to recreated: -bash-4.3$ cat test.js
var crypto = require('crypto');
crypto.randomBytes(0);
|
mscdex
added
crypto
|
Compiled with |
mhdawson
changed the title
|
@mhdawson I am not aware of any interception of malloc in v8_inspector, so this is curious. Let me know what you find out. |
|
iWuzHere is also having some trouble compiling on our local machines (although builds ok at least on community CI) so this should be a fun one. |
|
LMK how to help. |
|
@ofrobots thanks for the offer, I think we need to gather some additional info about the compile failure (may be the compiler crashing) and I'm also trying a build with --without-inspector to see if that resolves the test failures. Once we have some more info I'll reach out. |
|
Run here https://ci.nodejs.org/job/node-test-commit-aix-mdawson/9/ against PR that fixes --without-inspector to see if we still see failures if option is disabled, may help to focus on where to look if we don't see failures in that mode. |
|
Tests seem to have passed when compiled with --without-inspector so we need to look at the diffs between that being enabled/not |
|
Does something like this: Need to be added to inspector's gyp? Does running with inspector cause issues on any other platform like windows or solaris? |
|
@iwuzhere If AIX requires all C/C++ code to be compiled with those flags you mentioned, that maybe that's the problem. It is worth trying it out.
I haven't seen problems with this on Windows and on SmartOS, and neither has the CI. This does seem like something specific to AIX. /fyi @eugeneo. |
|
@iwuzhere could you try with this: https://github.com/eugeneo/node/commit/bf4cbfd64bde96fdf4ca9f33d65199d72f5b315b on an AIX machine? |
|
launched run here: |
|
Also trying to pare down where the issue might be I did a run with this change: diff --git a/node.gyp b/node.gyp
index 05a5530..11f696a 100644
--- a/node.gyp
+++ b/node.gyp
@@ -255,15 +255,6 @@
'HAVE_INSPECTOR=1',
'V8_INSPECTOR_USE_STL=1',
],
- 'sources': [
- 'src/inspector_agent.cc',
- 'src/inspector_socket.cc',
- 'src/inspector_socket.h',
- 'src/inspector-agent.h',
- ],
- 'dependencies': [
- 'deps/v8_inspector/v8_inspector.gyp:v8_inspector',
- ],
'include_dirs': [
'deps/v8_inspector',
'deps/v8_inspector/deps/wtf', # temporary
And the node exe built with that seems to pass the minimal test case without OOM |
|
So will be interested in run above, as it would seem to indicate that the other changes in node.gyp are not related. |
|
Looked at the differences in the compile line between the good/bad runs for node_crypto.cc but don't see anything obvious. The differences are those expected based on the changes to the gyp file and -D_LINUX_SOURCE_COMPAT is there in both cases. |
|
You dont just need I have already done a run with that change included in node.gyp but the tests still failed, hence the suggestion to try and add it to the inspectors gyp |
|
@iwuzhere Can you try this on AIX - https://github.com/eugeneo/node/commit/de97daf98de030fc697acd8bed627293caf32370 ? |
|
@eugeneo I have already done it locally and waiting for the compile to finish 😄 |
|
Still fail with changes to inspector.gyp |
|
Alright so did quite a bit of investigation ( still not sure why this is happening ). Checking symbols./configureTest fails ./configure --without-inspectorTest passes Stepping through codeDecided to step through code to see what was going on With InspectorEven though I break on Without InspectorMy breakpoint on Note we have a valid pointer in this case Disassembly of
|
|
So here's an objdump of node_crypto.o compile with and without the inspector Without InspectorWith Inspector |
|
So the issue seems to be the Set to 1Set to 0 |
|
Removing the following lines and compiling again with ChangesResultSo having an inspector agent causes us to use the wrong malloc |
|
I think this may have to do with the inspector_agent.h. That header uses std::vector. Could it be that it is STL that pulls in the wrong malloc? |
|
Looks like you're right |
|
Ok, I'll do a change tomorrow that removes the vector from the header. On Thu, Jun 2, 2016 at 1:29 PM Imran Iqbal notifications@github.com wrote:
|
|
I am a little bit confused, there are a lot of other files that also |
|
I think it might have to do with the inspector_socket.h being a header On Thu, Jun 2, 2016 at 1:42 PM Ali Ijaz Sheikh notifications@github.com
|
|
So this raises a few questions:
|
|
Can you try this - This just a proof of concept, actual implementation will be cleaner. On Thu, Jun 2, 2016 at 3:08 PM Imran Iqbal notifications@github.com wrote:
|
|
https://ci.nodejs.org/job/node-test-commit-aix/205/ |
|
@eugeneo that changeset appears to have fixed the issue |
|
Testing out the following changeset to see if it sovles the issue: https://ci.nodejs.org/job/node-test-commit-aix/nodes=aix61-ppc64/207/ |
|
Based on the investigation (myself and iWuzHere) our understanding is that the core issue is a problem with how _LINUX_SOURCE_COMPAT was implemented for AIX. We've talked to the team that handles gcc for AIX and it sounds like they will update the toolchain so that it defines malloc (and variants) in a better way that won't conflict with what the STL header was doing. This is however, a longer term solution as it will take a while for this to make it into the toolchain and us to upgrade to later levels of the compiler. In the shorter term it looks like we have 2 options. One is to refactor the inspector code to avoid the problem as @eugeneo has been working on. The second is to update the use of malloc (and variants) in Node as @iwuzhere has done in https://github.com/iWuzHere/node/commit/42e12bfcc9b7bd23f37fe37e8fe8d0d29e63e00e. I think I'd favor the second option if we can get it accepted since it would cover other similar issues if they arise before the toolchain is updated. @eugeneo and @ofrobots whats your take ? |
|
I think I'd like to refactor the inspector in either case, even if not for AIX issue. |
|
Ok so sounds like both would likely make sense. @iwuzhere can you submit the PR referencing this issue. I'd include the information in the pull request that the longer term change will be in the toolchain but this will make sure we don't see other similar issues up until the toolchain is updated. |
|
Created pull request #7229 |
This is needed to reduce the coupling between node files that use node::Environment and inspector class. Fixes: nodejs#7080
3 new failures in last nights run, opening as single issue until more investigation is completed.
https://ci.nodejs.org/job/node-test-commit-aix/nodes=aix61-ppc64/195/console
not ok 6 parallel/test-async-wrap-post-did-throw # FATAL ERROR: node::RandomBytesRequest() Out of Memory not ok 8 parallel/test-async-wrap-throw-from-callback # # assert.js:90 # throw new assert.AssertionError({ # ^ # AssertionError: pre closed with code null # at ChildProcess.child.on (/home/iojs/build/workspace/node-test-commit-aix/nodes/aix61-ppc64/test/parallel/test-async-wrap-throw-from-callback.js:65:14) # at emitTwo (events.js:106:13) # at ChildProcess.emit (events.js:191:7) # at maybeClose (internal/child_process.js:852:16) # at Process.ChildProcess._handle.onexit (internal/child_process.js:215:5) --- duration_ms: 0.978 ... not ok 177 parallel/test-crypto-random # FATAL ERROR: node::RandomBytesRequest() Out of MemoryThe text was updated successfully, but these errors were encountered: