Skip to content
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

Diagnostic report on fatal error does not respect settings #31576

Closed
brandondoran opened this issue Jan 30, 2020 · 4 comments
Closed

Diagnostic report on fatal error does not respect settings #31576

brandondoran opened this issue Jan 30, 2020 · 4 comments
Labels
report Issues and PRs related to process.report.

Comments

@brandondoran
Copy link

  • Version: v12.14.1, v12.14.0
  • Platform:
    • Darwin Kernel Version 18.7.0: Tue Aug 20 16:57:14 PDT 2019; root:xnu-4903.271.2~2/RELEASE_X86_64 x86_64
    • Linux 50565de9c35c 4.19.76-linuxkit deps: update openssl to 1.0.1j #1 SMP Thu Oct 17 19:31:58 UTC 2019 x86_64 Linux
  • Subsystem: reports

While troubleshooting OOM crashes in production, I wanted to get access to the diagnostic report. Because it's a container environment, I need the file to be written to a volume, not the cwd, so that it can be accessed when the container goes away. When specifying a report directory via the command line, such as --report-directory=/var/node, it is not respected. The file is always written to the default location, the cwd of the node process.

Upon further inspection, it appears that the report is generated, even if --report-on-fatalerror is not specified.

I stepped through the C++ code and it looks like Environment::GetCurrent(isolate) here is returning a null pointer.

Steps to reproduce

  1. Create reports sub dir where we'd like the report to be written on fatal error:
    $ mkdir reports
  2. Run node eval script that will trigger the OOM fatal error. Set cli flags to configure report:
    $ node --experimental-report --max-old-space-size=20 --report-on-fatalerror --report-directory="$(pwd)/reports" -e 'const list = []; while(true) { list.push( new Record()); } function Record() { this.name = "foo"; }'
    
    <--- Last few GCs --->
    
    [42964:0x108000000]      338 ms: Mark-sweep 36.1 (54.9) -> 35.0 (64.9) MB, 52.7 / 0.0 ms  (average mu = 0.150, current mu = 0.114) allocation failure scavenge might not succeed
    [42964:0x108000000]      395 ms: Mark-sweep 35.0 (64.9) -> 34.6 (67.4) MB, 56.4 / 0.0 ms  (average mu = 0.075, current mu = 0.000) allocation failure scavenge might not succeed
    
    
    <--- JS stacktrace --->
    
    ==== JS stack trace =========================================
    
        0: ExitFrame [pc: 0x1009311f9]
    Security context: 0x3e5cc99808a1 <JSObject>
        1: /* anonymous */ [0x3e5c40893e71] [[eval]:~1] [pc=0xafa9afc39e6](this=0x3e5c3a1c2541 <JSGlobal Object>)
        2: InternalFrame [pc: 0x1008aee3d]
        3: EntryFrame [pc: 0x1008aec18]
        4: builtin exit frame: runInThisContext(this=0x3e5c57c8d631 <ContextifyScript map = 0x3e5ca51fd5b1>,0x3e5ca4d806e1 <false>,0x3e5ca4d806e1 <false>,0x3e5ca4d80631 <true>,-1,0x3...
    
    FATAL ERROR: Ineffective mark-compacts near heap limit Allocation failed - JavaScript heap out of memory
    
    Writing Node.js report to file: report.20200128.203533.42964.0.001.json
  3. check reports dir. It's empty:
    $ ls reports
    
  4. check cwd. The report is there:
    $ ls report.*
    report.20200128.203533.42964.0.001.json
    
  5. Remove --report-on-fatalerror arg and re-run. The report will still be generated.
@richardlau richardlau added the report Issues and PRs related to process.report. label Jan 30, 2020
@richardlau
Copy link
Member

For some reason #29601 is 404 erroring but there is a known issue with fatal errors resulting in the report code not having access to the options as they're currently hung off env->isolate_data() which is not available on fatal errors (see nodejs/diagnostics#331 (comment)).

#29881 was fixing --report-on-fatalerror, but it looks like we may need to move more options from PerIsolateOptions to PerProcessOptions or have some other fallback mechanism when env (and env->isolate_data()) are unavailable as in the case of fatal errors.

cc @addaleax @gireeshpunathil @nodejs/diagnostics

@gireeshpunathil
Copy link
Member

#29881 is indeed an attempt to address this. Pinging @shobhitchittora to see where do we stand on this

@brandondoran
Copy link
Author

I ran across #29601 while searching for existing issue and it 404'ed for me too. I'm happy to try and help on this if needed.

@ghost
Copy link

ghost commented Feb 1, 2020

Unfortunately, the issue is probably not going to be available again. If anyone has saved the content of the issue feel free to post.

See details

image-github

HarshithaKP added a commit to HarshithaKP/node that referenced this issue Mar 24, 2020
--report-on-fatalerror was not honored properly, as there was no
way to check the value which was stored in the Environment pointer
which can be inaccessible under certain fatal error situations.

Move the flag out of Environment pointer so that this is doable.

Co-authored-by: Shobhit Chittora schittora@paypal.com

Fixes: nodejs#31576
Refs: nodejs#29881
MylesBorins pushed a commit that referenced this issue Mar 25, 2020
--report-on-fatalerror was not honored properly, as there was no
way to check the value which was stored in the Environment pointer
which can be inaccessible under certain fatal error situations.

Move the flag out of Environment pointer so that this is doable.

Co-authored-by: Shobhit Chittora schittora@paypal.com

PR-URL: #32207
Fixes: #31576
Refs: #29881
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
targos pushed a commit to targos/node that referenced this issue Apr 25, 2020
--report-on-fatalerror was not honored properly, as there was no
way to check the value which was stored in the Environment pointer
which can be inaccessible under certain fatal error situations.

Move the flag out of Environment pointer so that this is doable.

Co-authored-by: Shobhit Chittora schittora@paypal.com

PR-URL: nodejs#32207
Fixes: nodejs#31576
Refs: nodejs#29881
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
targos pushed a commit that referenced this issue Apr 28, 2020
--report-on-fatalerror was not honored properly, as there was no
way to check the value which was stored in the Environment pointer
which can be inaccessible under certain fatal error situations.

Move the flag out of Environment pointer so that this is doable.

Co-authored-by: Shobhit Chittora schittora@paypal.com

PR-URL: #32207
Fixes: #31576
Refs: #29881
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
report Issues and PRs related to process.report.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants