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

inspector: Do cleanups before notifying callback #7450

Closed
wants to merge 1 commit into from
Closed

inspector: Do cleanups before notifying callback #7450

wants to merge 1 commit into from

Conversation

eugeneo
Copy link
Contributor

@eugeneo eugeneo commented Jun 27, 2016

Checklist
  • make -j4 test (UNIX), or vcbuild test nosign (Windows) passes
  • tests and/or benchmarks are included
  • commit message follows commit guidelines
Affected core subsystem(s)

inspector: This change is to inspector only

Description of change

Inspector socket implementation was notifying the handshake callback before
performing the cleanups, which meant that callback could not reclaim
resources allocated by the client. New implementation will free all
resource not allocated by the client before calling the callback,
allowing the client to complete the cleanup.

Fixes: #7418

CC: @ofrobots

@nodejs-github-bot nodejs-github-bot added the c++ Issues and PRs that require attention from people who are familiar with C++. label Jun 27, 2016
@mscdex mscdex added the inspector Issues and PRs related to the V8 inspector protocol label Jun 28, 2016
static void report_handshake_failure_cb(uv_handle_t* handle) {
dispose_inspector(handle);
inspector_socket_t* inspector =
reinterpret_cast<inspector_socket_t*>(handle->data);
Copy link
Member

Choose a reason for hiding this comment

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

static_cast

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

Inspector socket implementation was notifying handshake callback before
performing the cleanups, which meant that callback could not reclaim
resources allocated by the client. New implementation will free all
resource not allocated by the client before calling the callback,
allowing the client to complete the cleanup.

Fixes: #7418
@eugeneo
Copy link
Contributor Author

eugeneo commented Jun 28, 2016

@bnoordhuis Thank you for the review. I updated the code, please take another look.

@bnoordhuis
Copy link
Member

Thanks, LGTM.

@ofrobots
Copy link
Contributor

@ofrobots
Copy link
Contributor

Landed as 08aff14.

@ofrobots ofrobots closed this Jun 29, 2016
ofrobots pushed a commit that referenced this pull request Jun 29, 2016
Inspector socket implementation was notifying handshake callback before
performing the cleanups, which meant that callback could not reclaim
resources allocated by the client. New implementation will free all
resource not allocated by the client before calling the callback,
allowing the client to complete the cleanup.

Fixes: #7418
PR-URL: #7450
Reviewed-By: bnoordhuis - Ben Noordhuis <info@bnoordhuis.nl>
Fishrock123 pushed a commit that referenced this pull request Jul 5, 2016
Inspector socket implementation was notifying handshake callback before
performing the cleanups, which meant that callback could not reclaim
resources allocated by the client. New implementation will free all
resource not allocated by the client before calling the callback,
allowing the client to complete the cleanup.

Fixes: #7418
PR-URL: #7450
Reviewed-By: bnoordhuis - Ben Noordhuis <info@bnoordhuis.nl>
@Fishrock123 Fishrock123 mentioned this pull request Jul 5, 2016
@eugeneo eugeneo deleted the npe branch August 2, 2016 23:46
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++. inspector Issues and PRs related to the V8 inspector protocol
Projects
None yet
Development

Successfully merging this pull request may close these issues.

node --inspect connecting with url in localhost:<port>/json gives malloc error with nightlies
6 participants