core: implement runtime flag to print warn on sync#1707
Uh oh!
There was an error while loading. Please reload this page.
Conversation
src/node.h Outdated
There was a problem hiding this comment.
@bnoordhuis I'm pretty sure node.h isn't the correct place for this, but wasn't sure where else to place it.
trevnorrisforce-pushed the flag-sync-methods branch 2 times, most recently from 8743a9f to 19e77d3Compare
ChALkeR commented May 14, 2015
Does this handle |
trevnorris commented May 15, 2015
@ChALkeR Thanks. Figured I'd missed some. I'll add that. |
trevnorrisforce-pushed the flag-sync-methods branch 4 times, most recently from 80926f2 to 066f492Comparetrevnorris commented May 15, 2015
@ChALkeR Added. @iojs/tc I've updated the implementation to store the needed value on |
trevnorrisforce-pushed the flag-sync-methods branch from 066f492 to d1e1744Compare
Fishrock123 commented May 15, 2015
I think this could be useful. 👍 |
src/env-inl.h Outdated
There was a problem hiding this comment.
Could we use Message::PrintCurrentStackTrace(isolate_, stderr) so that v8 does the formatting? Since it's pretty complicated when you start to handle evals, constructors etc.
There was a problem hiding this comment.
That would make things much easier. Though there's a slight style issue that doesn't sit well with me. Here's an example output:
WARNING: Detected use of sync API pbkdf2 (crypto.js:592:20) Object.exports.pbkdf2Sync (crypto.js:583:10) null._onTimeout (/tmp/x.js:8:10) Timer.listOnTimeout (timers.js:89:15) I would prefer the stack trace to look more like an error's stack trace with at at the beginning of each line. But reimplementing the entire formatting would be a pain. I have the change ready and waiting but would like your thoughts on this before I push it up.
There was a problem hiding this comment.
oh I expected it to be in same format as you would get when doing new Error().stack or Error.captureStackTrace in javascript. Another option would be to call into javascript to get a formatted stack trace.
There was a problem hiding this comment.
Yeah. I see how complex the formatting is for the stack trace in CallSiteToString() in deps/v8/src/message.js.
@bnoordhuis If we were to call to JS, what would be the best way to do that from an env function?
petkaantonov commented May 15, 2015
The synchronous code has been run after diff --git a/src/node.cc b/src/node.cc index 4cafe96..8dec8da 100644 --- a/src/node.cc+++ b/src/node.cc@@ -3887,6 +3887,9 @@ static void StartNodeInstance(void* arg){LoadEnvironment(env); + if (warn_on_sync)+ env->set_warn_on_sync(true);+ // Enable debugger if (instance_data->use_debug_agent()) EnableDebug(env); |
trevnorrisforce-pushed the flag-sync-methods branch from d1e1744 to 343fc0eComparetrevnorris commented May 15, 2015
Great idea. Implemented that change. |
trevnorris changed the title trevnorrisforce-pushed the flag-sync-methods branch 3 times, most recently from 8205884 to 347c94dComparesrc/node.cc Outdated
There was a problem hiding this comment.
It seems weird that this property is always either true or doesn't exist, should it just always be defined as either true or false?
There was a problem hiding this comment.
I'm just following the current implementation for all other properties of this type, e.g. process.traceDeprecation.
There was a problem hiding this comment.
though you're right. a PR to conditionally set those would be fairly trivial. i'll put that on my list, unless you get to it first. :)
petkaantonov commented May 15, 2015
LGTM with comments |
trevnorrisforce-pushed the flag-sync-methods branch from 347c94d to 7d1474eCompareChALkeR commented May 15, 2015
Cross-linking: #1674 |
petkaantonov commented May 15, 2015
Almost forgot, there should be |
trevnorrisforce-pushed the flag-sync-methods branch from 7d1474e to c6ea43dComparetrevnorris commented May 15, 2015
@petkaantonov Good call. Also I analyzed the stack trace formatting that V8 uses and matched it as closely as possible using the available C++ API. I think that will be good enough for now. |
Use the --trace-sync-io flag to print a stack trace whenever a sync method is used after the first tick, excluding during the process exit event. (e.g. fs.readFileSync()) It does not track if the warning has occurred at a specific location in the past and so will print the warning every time. Reason for not printing during the first tick of the appication is so all necessary resources can be required. Also by excluding synchronous calls during exit is necessary in case any data needs to be logged out by the application before it shuts down. Fixes: nodejs#1674 PR-URL: nodejs#1707 Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: Fedor Indutny <[email protected]> Reviewed-By: Jeremiah Senkpiel <[email protected]> Reviewed-By: Petka Antonov <[email protected]>
trevnorrisforce-pushed the flag-sync-methods branch from 2c715a6 to dd6f71aCompareUse the --trace-sync-io flag to print a stack trace whenever a sync method is used after the first tick, excluding during the process exit event. (e.g. fs.readFileSync()) It does not track if the warning has occurred at a specific location in the past and so will print the warning every time. Reason for not printing during the first tick of the appication is so all necessary resources can be required. Also by excluding synchronous calls during exit is necessary in case any data needs to be logged out by the application before it shuts down. Fixes: nodejs#1674 PR-URL: nodejs#1707 Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: Fedor Indutny <[email protected]> Reviewed-By: Jeremiah Senkpiel <[email protected]> Reviewed-By: Petka Antonov <[email protected]>
src/env.cc Outdated
There was a problem hiding this comment.
I wonder if we could use fn_name_s.length() here ;)
Use the --trace-sync-io flag to print a stack trace whenever a sync method is used after the first tick, excluding during the process exit event. (e.g. fs.readFileSync()) It does not track if the warning has occurred at a specific location in the past and so will print the warning every time. Reason for not printing during the first tick of the appication is so all necessary resources can be required. Also by excluding synchronous calls during exit is necessary in case any data needs to be logged out by the application before it shuts down. Fixes: nodejs#1674 PR-URL: nodejs#1707 Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: Fedor Indutny <[email protected]> Reviewed-By: Jeremiah Senkpiel <[email protected]> Reviewed-By: Petka Antonov <[email protected]>
trevnorrisforce-pushed the flag-sync-methods branch from dd6f71a to c1de6d2Compare
benjamingr commented May 18, 2015
Related to the code conversation - I think the performance of printing the trace is mostly irrelevant anyway @trevnorris - we expect this to log very rarely and never in production anyway. I'm not even sure that it's a bad idea to flush it at every iteration. Don't be so hard on yourself :) Related to the name - are we OK with On an unrelated hook - it might be interesting to find out if there is a need to expose sync io detected events on Good work by the way. |
indutny commented May 18, 2015
LGTM |
trevnorris commented May 18, 2015
@benjamingr What I was taking about is that I return if one of the stack trace lines was ever an It does only print after the first "phase" (not "tick" as in @indutny thanks! |
trevnorris commented May 18, 2015
Thanks all. Landed in c1de6d2. |
benjamingr commented May 18, 2015
Nice work, and thanks :) |
silverwind commented May 19, 2015
@trevnorris seems the new test added here fails on a few platforms: https://jenkins-iojs.nodesource.com/view/iojs/job/iojs+any-pr+multi/686/ |
santigimeno commented May 19, 2015
Tentative fix @ #1734 |
PR-URL: nodejs#1532 Notable Changes: * crypto: Diffie-Hellman key exchange (DHE) parameters ('dhparams') must now be 1024 bits or longer or an error will be thrown. A warning will also be printed to the console if you supply less than 2048 bits. See https://weakdh.org/ for further context on this security concern. (Shigeki Ohtsu) nodejs#1739. * node: A new --trace-sync-io command line flag will print a warning and a stack trace whenever a synchronous API is used. This can be used to track down synchronous calls that may be slowing down an application. (Trevor Norris) nodejs#1707. * node: To allow for chaining of methods, the setTimeout(), setKeepAlive(), setNoDelay(), ref() and unref() methods used in 'net', 'dgram', 'http', 'https' and 'tls' now return the current instance instead of undefined (Roman Reiss) nodejs#1779. * util: A significant speed-up (in the order of 35%) for the common-case of a single string argument to util.format(), used by console.log() (Сковорода Никита Андреевич) nodejs#1749.
PR-URL: nodejs#1532 Notable Changes: * crypto: Diffie-Hellman key exchange (DHE) parameters ('dhparams') must now be 1024 bits or longer or an error will be thrown. A warning will also be printed to the console if you supply less than 2048 bits. See https://weakdh.org/ for further context on this security concern. (Shigeki Ohtsu) nodejs#1739. * node: A new --trace-sync-io command line flag will print a warning and a stack trace whenever a synchronous API is used. This can be used to track down synchronous calls that may be slowing down an application. (Trevor Norris) nodejs#1707. * node: To allow for chaining of methods, the setTimeout(), setKeepAlive(), setNoDelay(), ref() and unref() methods used in 'net', 'dgram', 'http', 'https' and 'tls' now return the current instance instead of undefined (Roman Reiss & Evan Lucas) nodejs#1699nodejs#1768nodejs#1779. * util: A significant speed-up (in the order of 35%) for the common-case of a single string argument to util.format(), used by console.log() (Сковорода Никита Андреевич) nodejs#1749.
PR-URL: nodejs#1532 Notable Changes: * crypto: Diffie-Hellman key exchange (DHE) parameters ('dhparams') must now be 1024 bits or longer or an error will be thrown. A warning will also be printed to the console if you supply less than 2048 bits. See https://weakdh.org/ for further context on this security concern. (Shigeki Ohtsu) nodejs#1739. * node: A new --trace-sync-io command line flag will print a warning and a stack trace whenever a synchronous API is used. This can be used to track down synchronous calls that may be slowing down an application. (Trevor Norris) nodejs#1707. * node: To allow for chaining of methods, the setTimeout(), setKeepAlive(), setNoDelay(), ref() and unref() methods used in 'net', 'dgram', 'http', 'https' and 'tls' now return the current instance instead of undefined (Roman Reiss & Evan Lucas) nodejs#1699nodejs#1768nodejs#1779. * util: A significant speed-up (in the order of 35%) for the common-case of a single string argument to util.format(), used by console.log() (Сковорода Никита Андреевич) nodejs#1749.
PR-URL: nodejs#1532 Notable Changes: * crypto: Diffie-Hellman key exchange (DHE) parameters ('dhparams') must now be 1024 bits or longer or an error will be thrown. A warning will also be printed to the console if you supply less than 2048 bits. See https://weakdh.org/ for further context on this security concern. (Shigeki Ohtsu) nodejs#1739. * node: A new --trace-sync-io command line flag will print a warning and a stack trace whenever a synchronous API is used. This can be used to track down synchronous calls that may be slowing down an application. (Trevor Norris) nodejs#1707. * node: To allow for chaining of methods, the setTimeout(), setKeepAlive(), setNoDelay(), ref() and unref() methods used in 'net', 'dgram', 'http', 'https' and 'tls' now return the current instance instead of undefined (Roman Reiss & Evan Lucas) nodejs#1699nodejs#1768nodejs#1779. * npm: Upgraded to v2.10.1, release notes can be found in https://github.com/npm/npm/releases/tag/v2.10.1 and https://github.com/npm/npm/releases/tag/v2.10.0. * util: A significant speed-up (in the order of 35%) for the common-case of a single string argument to util.format(), used by console.log() (Сковорода Никита Андреевич) nodejs#1749.
PR-URL: #1777 Notable Changes: * crypto: Diffie-Hellman key exchange (DHE) parameters ('dhparams') must now be 1024 bits or longer or an error will be thrown. A warning will also be printed to the console if you supply less than 2048 bits. See https://weakdh.org/ for further context on this security concern. (Shigeki Ohtsu) #1739. * node: A new --trace-sync-io command line flag will print a warning and a stack trace whenever a synchronous API is used. This can be used to track down synchronous calls that may be slowing down an application. (Trevor Norris) #1707. * node: To allow for chaining of methods, the setTimeout(), setKeepAlive(), setNoDelay(), ref() and unref() methods used in 'net', 'dgram', 'http', 'https' and 'tls' now return the current instance instead of undefined (Roman Reiss & Evan Lucas) #1699#1768#1779. * npm: Upgraded to v2.10.1, release notes can be found in https://github.com/npm/npm/releases/tag/v2.10.1 and https://github.com/npm/npm/releases/tag/v2.10.0. * util: A significant speed-up (in the order of 35%) for the common-case of a single string argument to util.format(), used by console.log() (Сковорода Никита Андреевич) #1749.
Use the --trace-sync-io flag to print a stack trace whenever a sync method is used after the first tick, excluding during the process exit event. (e.g. fs.readFileSync()) It does not track if the warning has occurred at a specific location in the past and so will print the warning every time. Reason for not printing during the first tick of the appication is so all necessary resources can be required. Also by excluding synchronous calls during exit is necessary in case any data needs to be logged out by the application before it shuts down. Fixes: nodejs/node#1674 PR-URL: nodejs/node#1707 Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: Fedor Indutny <[email protected]> Reviewed-By: Jeremiah Senkpiel <[email protected]> Reviewed-By: Petka Antonov <[email protected]>
11 participants
Use the --warn-on-sync flag to print a stack trace whenever a sync
method is used. (e.g. fs.readFileSync()) It does not track if the
warning has occurred as a specific location in the past and so will
print the warning every time the function is used.
This does not print the warning for the first synchronous execution of
the script. This will allow all the require(), etc. statements to run
that are necessary to prepare the environment.
NOTE: I believe I've addressed all applicable sync calls that should warn the user. I will not be enabling the warning for the
'net'module's.listen()calls that exclude the callback. Because the calls are actually synchronous under the hood and we simply queue the callback asynchronously to be run. It also appears to be the same forhttp.{get,request}(). Also calls to any stream's.write()are still async even if the callback is excluded.Opening this up now so we can have further discussion.