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

http: remove duplicate async_hooks init for http parser #23263

Conversation

basti1302
Copy link

@basti1302 basti1302 commented Oct 4, 2018

Each time a new parser was created, AsyncReset was being called via
the C++ Parser class constructor (super constructor AsyncWrap) and also
via Parser::Reinitialize.

This also adds a missing async_hooks destroy event before AsyncReset is
called for the parser reuse case, otherwise the old async_id never gets
destroyed.

Refs: #19859

See also: #23201

Checklist

I was not yet able to write a decent test case for that, but the changes to test/async-hooks/test-graph.http.js already prove that the duplicated init hook call (which was actually tested for there) has been removed. I see what I can do about adding a more explicit test.

Update: There is a test now but it only works when I restrict it to the init hooks called in the http client context. Without this restriction, there are still init calls without a matching destroy so I suspect there is still in issue when using parsers in http server.

Update 2: I was able to remove the restriction on _http_client init calls, it was a test issue after all.

@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. http Issues or PRs related to the http subsystem. http_parser Issues and PRs related to the HTTP Parser dependency or the http_parser binding. labels Oct 4, 2018
triggerAsyncId: 'tcp:2' },
{ type: 'Timeout',
id: 'timeout:2',
triggerAsyncId: 'httpparser:4' },
triggerAsyncId: 'httpparser:2' },
{ type: 'SHUTDOWNWRAP',
id: 'shutdown:1',
triggerAsyncId: 'tcp:2' } ]
Copy link
Author

@basti1302 basti1302 Oct 4, 2018

Choose a reason for hiding this comment

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

This very much looks like the output of verify-graph#printGraph has simply been copied here without giving it much thought. The duplicated HTTPPARSER init calls were showing here and don't make much sense, IMO.

@@ -8,24 +8,23 @@ const FreeList = require('internal/freelist');

assert.strictEqual(typeof FreeList, 'function');

const flist1 = new FreeList('flist1', 3, String);
const flist1 = new FreeList('flist1', 3, Object);
Copy link
Author

Choose a reason for hiding this comment

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

One downside of this change is that the items in freelist can no longer be primitives, since we attach an attribute to it. But as the freelist is only used for HTTP parsers anyway, I don't think it is an issue.

@AyushG3112
Copy link
Contributor

cc @nodejs/http @nodejs/async_hooks

@basti1302 basti1302 force-pushed the fix-missing-async-hooks-destroy-http-parser branch 2 times, most recently from db9426d to 0ba5ae7 Compare October 4, 2018 17:17
@@ -23,4 +23,14 @@ class FreeList {
}
}

function needsToCallAsyncReset(item) {
item.needsAsyncReset = true;
Copy link
Contributor

Choose a reason for hiding this comment

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

Perhaps use a symbol?

Copy link
Author

Choose a reason for hiding this comment

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

Will do.

Copy link
Author

Choose a reason for hiding this comment

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

Done.

@ofrobots
Copy link
Contributor

ofrobots commented Oct 4, 2018

/cc @mcollina and @kjin as well as they are not in the async-hooks team.

@basti1302 basti1302 force-pushed the fix-missing-async-hooks-destroy-http-parser branch 2 times, most recently from e27f470 to d35be44 Compare October 4, 2018 20:08
Each time a new parser was created, AsyncReset was being called via
the C++ Parser class constructor (super constructor AsyncWrap) and also
via Parser::Reinitialize.

This also adds a missing async_hooks destroy event before AsyncReset is
called for the parser reuse case, otherwise the old async_id never gets
destroyed.

Refs: nodejs#19859
This also at least keeps up the appearance of FreeList being a generic
all purpose thing (right now it is only used for HTTP parsers but
theoretically it could be used for any reusable resource). The
previous name "needsAsyncReset" implied a tight coupling to to async
resources, the new name "is_reused" is more generic.
@basti1302 basti1302 force-pushed the fix-missing-async-hooks-destroy-http-parser branch from 0c081fc to 26d1471 Compare October 4, 2018 20:46
@basti1302
Copy link
Author

Superseded by #23272 due to feedback in #23201.

@basti1302 basti1302 closed this Oct 5, 2018
@basti1302 basti1302 deleted the fix-missing-async-hooks-destroy-http-parser branch October 10, 2018 08:15
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++. http_parser Issues and PRs related to the HTTP Parser dependency or the http_parser binding. http Issues or PRs related to the http subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants