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

async-hooks: internalize setInitTriggerId #14302

Closed

Conversation

refack
Copy link
Contributor

@refack refack commented Jul 16, 2017

  • replace setInitTriggerId with triggerIdScopeSync
Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines
Affected core subsystem(s)

async_hooks

@nodejs-github-bot nodejs-github-bot added the lib / src Issues and PRs related to general changes in the lib or src directory. label Jul 16, 2017
@refack refack self-assigned this Jul 16, 2017
@refack refack added async_hooks Issues and PRs related to the async hooks subsystem. wip Issues and PRs that are still a work in progress. labels Jul 16, 2017
@refack
Copy link
Contributor Author

refack commented Jul 16, 2017

/cc @nodejs/async_hooks

I'm still getting this wrong with 605de08, it does not detect the retrive & reset

not ok 23 async-hooks/test-graph.pipeconnect
  ---
  duration_ms: 0.511
  severity: fail
  stack: |-
    'pipeconnect:1' expected to be triggered by 'pipe:2', but was triggered by 'pipe:1' instead.
    assert.js:48
      throw new errors.AssertionError({
      ^
    
    AssertionError [ERR_ASSERTION]: 1 === 0
        at verifyGraph (/data/iojs/build/workspace/node-test-commit-linuxone/nodes/rhel72-s390x/test/async-hooks/verify-graph.js:89:10)
        at process.onexit (/data/iojs/build/workspace/node-test-commit-linuxone/nodes/rhel72-s390x/test/async-hooks/test-graph.pipeconnect.js:29:3)
        at emitOne (events.js:120:20)
        at process.emit (events.js:210:7)
  ...
not ok 25 async-hooks/test-graph.shutdown
  ---
  duration_ms: 0.510
  severity: fail
  stack: |-
    'getaddrinforeq:1' expected to be triggered by 'tcp:2', but was triggered by 'tcp:1' instead.
    assert.js:48
      throw new errors.AssertionError({
      ^
    
    AssertionError [ERR_ASSERTION]: 1 === 0
        at verifyGraph (/data/iojs/build/workspace/node-test-commit-linuxone/nodes/rhel72-s390x/test/async-hooks/verify-graph.js:89:10)
        at process.onexit (/data/iojs/build/workspace/node-test-commit-linuxone/nodes/rhel72-s390x/test/async-hooks/test-graph.shutdown.js:35:3)
        at emitOne (events.js:120:20)
        at process.emit (events.js:210:7)
  ...
not ok 30 async-hooks/test-graph.tls-write
  ---
  duration_ms: 0.512
  severity: fail
  stack: |-
    'getaddrinforeq:1' expected to be triggered by 'tls:1', but was triggered by 'tcp:1' instead.
    assert.js:48
      throw new errors.AssertionError({
      ^
    
    AssertionError [ERR_ASSERTION]: 1 === 0
        at verifyGraph (/data/iojs/build/workspace/node-test-commit-linuxone/nodes/rhel72-s390x/test/async-hooks/verify-graph.js:89:10)
        at process.onexit (/data/iojs/build/workspace/node-test-commit-linuxone/nodes/rhel72-s390x/test/async-hooks/test-graph.tls-write.js:56:3)
        at emitOne (events.js:120:20)
        at process.emit (events.js:210:7)
  ...

@AndreasMadsen
Copy link
Member

AndreasMadsen commented Jul 16, 2017

I think we should wait with actual code until after we have finished all three discussions outlined in #14238. After that, it will be easier for us to decide on the correct layer of separation.

edit: I would also like the code separation/depreciation to happen separately from things such as the triggerIdScopeSync refactor. Otherwise, I think the PRs will become too big.

@refack
Copy link
Contributor Author

refack commented Jul 16, 2017

This PR is my code-and-learn... I personally think better through code... I won't mind if this PR will be closed without landing.

So I agree with your second point, re separation/depreciation/refactor

@AndreasMadsen
Copy link
Member

AndreasMadsen commented Jul 16, 2017

@refack I have created a private PR (https://github.com/AndreasMadsen/node/pull/1) that contains the separation and depreciation I have in mind. I will be happy to discuss refactors such as triggerIdScopeSync from that point of view.


I intend to submit a PR similar to https://github.com/AndreasMadsen/node/pull/1 if all three discussion topics as outlined in #14238 ends with us agreeing to deprecate the undocumented API. I'm not entirely convinced we will agree on that and I want to respect the decision process, this is why I'm so careful about discussing too many things at once.

Copy link
Member

@AndreasMadsen AndreasMadsen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just to avoid confusion, I'm -1 with the current approach but I support the overall idea.

@refack
Copy link
Contributor Author

refack commented Jul 17, 2017

Just to avoid confusion, I'm -1 with the current approach but I support the overall idea.

👍

@BridgeAR
Copy link
Member

Where do we stand here? Is this something you still want to follow up upon @refack? Due to the -1 I am not sure how much progress is possible here in general at the moment.

@BridgeAR BridgeAR added the stalled Issues and PRs that are stalled. label Sep 12, 2017
@AndreasMadsen
Copy link
Member

It is now clear to me that this exact implementation is not the solution. #14238 is a long discussion on some alternatives.

@refack refack closed this Sep 13, 2017
@refack
Copy link
Contributor Author

refack commented Sep 13, 2017

(this was a test balloon anyway)

@refack refack added invalid Issues and PRs that are invalid. and removed dont-land-on-v6.x wip Issues and PRs that are still a work in progress. stalled Issues and PRs that are stalled. labels Sep 13, 2017
@refack refack removed their assignment Oct 20, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
async_hooks Issues and PRs related to the async hooks subsystem. invalid Issues and PRs that are invalid. lib / src Issues and PRs related to general changes in the lib or src directory.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants