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

node: call set_trace_sync_io before bootstrap #5964

Closed
wants to merge 1 commit into from

Conversation

joshgav
Copy link
Contributor

@joshgav joshgav commented Mar 30, 2016

Pull Request check-list

  • Does make -j8 test (UNIX) or vcbuild test nosign (Windows) pass with
    this change (including linting)?
  • Is the commit message formatted according to [CONTRIBUTING.md][0]?
  • If this change fixes a bug (or a performance problem), is a regression
    test (or a benchmark) included?

Affected core subsystem(s)

node --trace-sync-io

Description of change

env->set_trace_sync_io was previously being called after node::LoadEnvironment so it wasn't in effect for the initial script.

env->set_trace_sync_io was previously being called after
node::LoadEnvironment, meaning it wasn't in effect for the initial
script.
@mscdex mscdex added the lib / src Issues and PRs related to general changes in the lib or src directory. label Mar 30, 2016
@joshgav
Copy link
Contributor Author

joshgav commented Mar 30, 2016

Save the following in a script and call with node --trace-sync-io script.js. Without this fix no traces are output, with fix they are. This will only effect the first turn, prior to any uv_run's, which may be why it doesn't seem to effect node -e.

var fs = require('fs')
console.log(fs.readFileSync(__filename, {encoding:'UTF8'}))

I'll add a test.

/cc @cjihrig

@cjihrig
Copy link
Contributor

cjihrig commented Mar 31, 2016

@trevnorris --trace-sync-io is supposed to allow sync calls on the first tick, right? I thought it was, but wanted to double check. The reasoning being that the first tick is usually application initialization.

@evanlucas
Copy link
Contributor

Yea, I think that was by design.

@Fishrock123
Copy link
Contributor

@cjihrig correct.

@joshgav
Copy link
Contributor Author

joshgav commented Mar 31, 2016

Okay, I'll add a comment to that effect then.

@Fishrock123 Fishrock123 added the c++ Issues and PRs that require attention from people who are familiar with C++. label Mar 31, 2016
@joshgav
Copy link
Contributor Author

joshgav commented Mar 31, 2016

On second thought, @trevnorris could you explain why we skip the trace on the initial tick? I can add a comment and check the docs, would be nice to have an explanation.

@trevnorris
Copy link
Contributor

First, yes, it is supposed to run after bootstrap. From node -h:

  --trace-sync-io       show stack trace when use of sync IO
                        is detected after the first tick

Reasons for this:

  1. require() is a sync I/O operation. So we would have to special case this in order to not print a warning for every use of require().
  2. It is common practice to load resources synchronously during bootstrap so that they are available when/if other modules require them.

Have you tried running ./node --trace-sync-io script.js where the script is an empty file? You'll see that there's a lot of output.

@Fishrock123 Fishrock123 added the wontfix Issues that will not be fixed. label Mar 31, 2016
@Fishrock123
Copy link
Contributor

Closing since this does not appear to be an issue, please let me know if this wasn't correct. :)

@Fishrock123 Fishrock123 closed this Apr 4, 2016
@joshgav joshgav deleted the trace-sync-io-fix branch May 9, 2016 17:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs related to general changes in the lib or src directory. wontfix Issues that will not be fixed.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants