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

Revert "src: remove trace_sync_io_ from env" #28926

Closed
wants to merge 1 commit into from

Conversation

@ChALkeR
Copy link
Member

commented Aug 1, 2019

This reverts commit 7fa5f54.

Fixes: #28913

7fa5f54 breaks the logic behind --trace-sync-io, it should be enabled only at a certain point in time, while that commit enables it from the very start, causing the issue linked above.

Or am I missing something?

Refs: https://nodejs.org/api/cli.html#cli_trace_sync_io

Prints a stack trace whenever synchronous I/O is detected after the first turn of the event loop.

Refs: #22726

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • commit message follows commit guidelines
@cjihrig

This comment has been minimized.

Copy link
Contributor

commented Aug 1, 2019

Is it possible to add a regression test?

@addaleax
Copy link
Member

left a comment

Thank you for catching this!

@ChALkeR

This comment has been minimized.

Copy link
Member Author

commented Aug 1, 2019

@cjihrig I will try to do that later this week, but I don't think that it is required in order to do a revert.

@Trott
Trott approved these changes Aug 1, 2019
@cjihrig
cjihrig approved these changes Aug 1, 2019
@jasnell
jasnell approved these changes Aug 1, 2019
@nodejs-github-bot

This comment has been minimized.

@nodejs-github-bot

This comment has been minimized.

Revert "src: remove trace_sync_io_ from env"
This reverts commit 7fa5f54.

The reverted commit breaks the logic behind --trace-sync-io, it should
be enabled only at a certain point in time, while that commit enables
it from the very start, causing warnings be printed for all sync io
instead of sync io after the first tick of the event loop as documented.

Fixes: #28913
Refs: #22726
Refs: https://nodejs.org/api/cli.html#cli_trace_sync_io
PR-URL: #28926

@ChALkeR ChALkeR force-pushed the ChALkeR:revert-7fa5f54e6f branch from 31cbdd7 to 31973cc Aug 2, 2019

@ChALkeR

This comment has been minimized.

Copy link
Member Author

commented Aug 2, 2019

Force-push just now does not change the code, just altered the commit message to add an explanation and refs.

@ChALkeR ChALkeR referenced this pull request Aug 2, 2019
0 of 4 tasks complete
@nodejs-github-bot

This comment has been minimized.

@Trott

This comment has been minimized.

Copy link
Member

commented Aug 4, 2019

Landed in aa252eb

@Trott Trott closed this Aug 4, 2019

Trott added a commit to Trott/io.js that referenced this pull request Aug 4, 2019
Revert "src: remove trace_sync_io_ from env"
This reverts commit 7fa5f54.

The reverted commit breaks the logic behind --trace-sync-io, it should
be enabled only at a certain point in time, while that commit enables
it from the very start, causing warnings be printed for all sync io
instead of sync io after the first tick of the event loop as documented.

Fixes: nodejs#28913
Refs: nodejs#22726
Refs: https://nodejs.org/api/cli.html#cli_trace_sync_io
PR-URL: nodejs#28926
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>

@ChALkeR ChALkeR deleted the ChALkeR:revert-7fa5f54e6f branch Aug 4, 2019

BridgeAR added a commit that referenced this pull request Aug 6, 2019
Revert "src: remove trace_sync_io_ from env"
This reverts commit 7fa5f54.

The reverted commit breaks the logic behind --trace-sync-io, it should
be enabled only at a certain point in time, while that commit enables
it from the very start, causing warnings be printed for all sync io
instead of sync io after the first tick of the event loop as documented.

Fixes: #28913
Refs: #22726
Refs: https://nodejs.org/api/cli.html#cli_trace_sync_io
PR-URL: #28926
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@BridgeAR BridgeAR referenced this pull request Aug 6, 2019
JeniaBR added a commit to JeniaBR/node that referenced this pull request Sep 11, 2019
Revert "src: remove trace_sync_io_ from env"
This reverts commit 7fa5f54.

The reverted commit breaks the logic behind --trace-sync-io, it should
be enabled only at a certain point in time, while that commit enables
it from the very start, causing warnings be printed for all sync io
instead of sync io after the first tick of the event loop as documented.

Fixes: nodejs#28913
Refs: nodejs#22726
Refs: https://nodejs.org/api/cli.html#cli_trace_sync_io
PR-URL: nodejs#28926
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
JeniaBR added a commit to JeniaBR/node that referenced this pull request Sep 11, 2019
Revert "src: remove trace_sync_io_ from env"
This reverts commit 7fa5f54.

The reverted commit breaks the logic behind --trace-sync-io, it should
be enabled only at a certain point in time, while that commit enables
it from the very start, causing warnings be printed for all sync io
instead of sync io after the first tick of the event loop as documented.

Fixes: nodejs#28913
Refs: nodejs#22726
Refs: https://nodejs.org/api/cli.html#cli_trace_sync_io
PR-URL: nodejs#28926
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants
You can’t perform that action at this time.