GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — n-api: detect deadlocks in thread-safe function by gabrielschulhof · Pull Request #32689 · nodejs/node

Conversation

@gabrielschulhof

@gabrielschulhof gabrielschulhof commented Apr 6, 2020 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor

We introduce status napi_would_deadlock to be used as a return status
by napi_call_threadsafe_function if the call is made with
napi_tsfn_blocking on the main thread and the queue is full.

Fixes: #32615
Signed-off-by: @gabrielschulhof

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
himself65 reacted with thumbs up emoji josephg reacted with heart emoji bricss reacted with eyes emoji
All reactions
  • 👍 1 reaction
  • ❤️ 1 reaction
  • 👀 1 reaction
@gabrielschulhof gabrielschulhof added semver-minor PRs that contain new features and should be released in the next minor version. node-api Issues and PRs related to Node-API. labels Apr 6, 2020
@gabrielschulhof
gabrielschulhof requested review from addaleax, legendecas and mhdawson April 6, 2020 17:45
@nodejs-github-bot nodejs-github-bot added the c++ Issues and PRs that require attention from people who are familiar with C++. label Apr 6, 2020
@gabrielschulhof
gabrielschulhof force-pushed the tsfn-avoid-deadlock branch from 968f06d to f93f12e Compare April 6, 2020 17:47
@gabrielschulhof
gabrielschulhof force-pushed the tsfn-avoid-deadlock branch from f93f12e to 7b258a1 Compare April 6, 2020 17:47
@nodejs-github-bot

nodejs-github-bot commented Apr 6, 2020

Copy link
Copy Markdown
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/30503/
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
addaleax
addaleax approved these changes Apr 6, 2020
View reviewed changes
Comment thread src/node_api.cc Outdated

@addaleax addaleax Apr 6, 2020

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Suggested change
uv_thread_t the_thread = uv_thread_self();
uv_thread_t current_thread = uv_thread_self();

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread doc/api/n-api.md Outdated

@addaleax addaleax Apr 6, 2020

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Add a changes: entry for napi_create_threadsafe_function below?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@gabrielschulhof

gabrielschulhof commented Apr 6, 2020

Copy link
Copy Markdown
Contributor Author

@addaleax I updated the variable name, added the changes: to napi_call_threadsafe_function() and re-worder the documentation about its return value a bit.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@nodejs-github-bot

nodejs-github-bot commented Apr 6, 2020

Copy link
Copy Markdown
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/30516/
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@nodejs-github-bot

nodejs-github-bot commented Apr 6, 2020 •
edited by gabrielschulhof
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/30517/ ✔️
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
jasnell
jasnell approved these changes Apr 7, 2020
View reviewed changes
legendecas
legendecas approved these changes Apr 7, 2020
View reviewed changes
Comment thread doc/api/n-api.md Outdated

@legendecas legendecas Apr 7, 2020

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Suggested change
description: Return `napi_would_deadlock` when called from the main thread.
description: Return `napi_would_deadlock` if called with `napi_tsfn_blocking` from the main thread and the queue is full.

Just another clarification...

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
devnexen
devnexen approved these changes Apr 7, 2020
View reviewed changes
cjihrig
cjihrig approved these changes Apr 7, 2020
View reviewed changes
mhdawson
mhdawson approved these changes Apr 7, 2020
View reviewed changes

@mhdawson mhdawson left a comment

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

LGTM

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@gabrielschulhof
gabrielschulhof force-pushed the tsfn-avoid-deadlock branch from 439af5e to 6322b5d Compare April 9, 2020 07:04
@nodejs-github-bot

nodejs-github-bot commented Apr 9, 2020

Copy link
Copy Markdown
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/30574/
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@gabrielschulhof
gabrielschulhof force-pushed the tsfn-avoid-deadlock branch from 6322b5d to 929cdb4 Compare April 9, 2020 07:09
@gabrielschulhof
gabrielschulhof force-pushed the tsfn-avoid-deadlock branch from 929cdb4 to 75b2509 Compare April 9, 2020 07:10
Gabriel Schulhof
n-api: detect deadlocks in thread-safe function
e5c61e8 
We introduce status `napi_would_deadlock` to be used as a return status
by `napi_call_threadsafe_function` if the call is made with
`napi_tsfn_blocking` on the main thread and the queue is full.

PR-URL: nodejs#32689
Fixes: nodejs#32615
Signed-off-by: Gabriel Schulhof <[email protected]>
@gabrielschulhof
gabrielschulhof force-pushed the tsfn-avoid-deadlock branch from 75b2509 to e5c61e8 Compare April 9, 2020 07:13
@nodejs-github-bot

nodejs-github-bot commented Apr 9, 2020

Copy link
Copy Markdown
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/30575/
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@nodejs-github-bot

nodejs-github-bot commented Apr 9, 2020 •
edited by gabrielschulhof
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/30577/ ✔️
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@gabrielschulhof

gabrielschulhof commented Apr 9, 2020

Copy link
Copy Markdown
Contributor Author

Landed in aeb7084.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@gabrielschulhof gabrielschulhof closed this Apr 9, 2020
gabrielschulhof pushed a commit that referenced this pull request Apr 9, 2020
Gabriel Schulhof
n-api: detect deadlocks in thread-safe function
aeb7084 
We introduce status `napi_would_deadlock` to be used as a return status
by `napi_call_threadsafe_function` if the call is made with
`napi_tsfn_blocking` on the main thread and the queue is full.

PR-URL: #32689
Fixes: #32615
Signed-off-by: Gabriel Schulhof <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: David Carlier <[email protected]>
Reviewed-By: Chengzhong Wu <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
targos pushed a commit that referenced this pull request Apr 12, 2020
Gabriel Schulhof @targos
n-api: detect deadlocks in thread-safe function
3907de7 
We introduce status `napi_would_deadlock` to be used as a return status
by `napi_call_threadsafe_function` if the call is made with
`napi_tsfn_blocking` on the main thread and the queue is full.

PR-URL: #32689
Fixes: #32615
Signed-off-by: Gabriel Schulhof <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: David Carlier <[email protected]>
Reviewed-By: Chengzhong Wu <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
@targos targos mentioned this pull request Apr 13, 2020
v13.13.0 release proposal #32813
Merged
@gabrielschulhof
gabrielschulhof deleted the tsfn-avoid-deadlock branch April 13, 2020 18:34
BethGriggs pushed a commit that referenced this pull request Apr 14, 2020
Gabriel Schulhof @BethGriggs
n-api: detect deadlocks in thread-safe function
45676be 
We introduce status `napi_would_deadlock` to be used as a return status
by `napi_call_threadsafe_function` if the call is made with
`napi_tsfn_blocking` on the main thread and the queue is full.

PR-URL: #32689
Fixes: #32615
Signed-off-by: Gabriel Schulhof <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: David Carlier <[email protected]>
Reviewed-By: Chengzhong Wu <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
targos added a commit that referenced this pull request Apr 14, 2020
@targos
2020-04-14, Version 13.13.0 (Current)
4ca9571 
Notable changes:

New file system APIs:
* Added a new function, `fs.readv` (with sync and promisified versions).
  This function takes an array of `ArrayBufferView` elements and will
  write the data it reads sequentially to the buffers
  (Sk Sajidul Kadir). #32356
* A new overload is available for `fs.readSync`, which allows to
  optionally pass any of the `offset`, `length` and `position`
  parameters. #32460

Other changes:
* dns:
  * Added the `dns.ALL` flag, that can be passed to `dns.lookup()` with
    `dns.V4MAPPED` to return resolved IPv6 addresses as well as IPv4
    mapped IPv6 addresses (murgatroid99).
    #32183
* http:
  * The default maximum HTTP header size was changed from 8KB to 16KB
    (rosaxny). #32520
* n-api:
  * Calls to `napi_call_threadsafe_function` from the main thread can
    now return the `napi_would_deadlock` status in certain
    circumstances (Gabriel Schulhof).
    #32689
* util:
  * Added a new `maxStrLength` option to `util.inspect`, to control the
    maximum length of printed strings. Its default value is `Infinity`
    (rosaxny). #32392
* worker:
  * Added support for passing a `transferList` along with `workerData`
    to the `Worker` constructor (Juan José Arboleda).
    #32278

New core collaborators:
With this release, we welcome three new Node.js core collaborators:
* himself65. #32734
* flarna (Gerhard Stoebich). #32620
* mildsunrise (Alba Mendez). #32525

PR-URL: #32813
targos added a commit that referenced this pull request Apr 14, 2020
@targos
2020-04-14, Version 13.13.0 (Current)
8130521 
Notable changes:

New file system APIs:
* Added a new function, `fs.readv` (with sync and promisified versions).
  This function takes an array of `ArrayBufferView` elements and will
  write the data it reads sequentially to the buffers
  (Sk Sajidul Kadir). #32356
* A new overload is available for `fs.readSync`, which allows to
  optionally pass any of the `offset`, `length` and `position`
  parameters. #32460

Other changes:
* dns:
  * Added the `dns.ALL` flag, that can be passed to `dns.lookup()` with
    `dns.V4MAPPED` to return resolved IPv6 addresses as well as IPv4
    mapped IPv6 addresses (murgatroid99).
    #32183
* http:
  * The default maximum HTTP header size was changed from 8KB to 16KB
    (rosaxny). #32520
* n-api:
  * Calls to `napi_call_threadsafe_function` from the main thread can
    now return the `napi_would_deadlock` status in certain
    circumstances (Gabriel Schulhof).
    #32689
* util:
  * Added a new `maxStrLength` option to `util.inspect`, to control the
    maximum length of printed strings. Its default value is `Infinity`
    (rosaxny). #32392
* worker:
  * Added support for passing a `transferList` along with `workerData`
    to the `Worker` constructor (Juan José Arboleda).
    #32278

New core collaborators:
With this release, we welcome three new Node.js core collaborators:
* himself65. #32734
* flarna (Gerhard Stoebich). #32620
* mildsunrise (Alba Mendez). #32525

PR-URL: #32813
targos added a commit that referenced this pull request Apr 14, 2020
@targos
2020-04-14, Version 13.13.0 (Current)
cf4c332 
Notable changes:

New file system APIs:
* Added a new function, `fs.readv` (with sync and promisified versions).
  This function takes an array of `ArrayBufferView` elements and will
  write the data it reads sequentially to the buffers
  (Sk Sajidul Kadir). #32356
* A new overload is available for `fs.readSync`, which allows to
  optionally pass any of the `offset`, `length` and `position`
  parameters. #32460

Other changes:
* dns:
  * Added the `dns.ALL` flag, that can be passed to `dns.lookup()` with
    `dns.V4MAPPED` to return resolved IPv6 addresses as well as IPv4
    mapped IPv6 addresses (murgatroid99).
    #32183
* http:
  * The default maximum HTTP header size was changed from 8KB to 16KB
    (rosaxny). #32520
* n-api:
  * Calls to `napi_call_threadsafe_function` from the main thread can
    now return the `napi_would_deadlock` status in certain
    circumstances (Gabriel Schulhof).
    #32689
* util:
  * Added a new `maxStrLength` option to `util.inspect`, to control the
    maximum length of printed strings. Its default value is `Infinity`
    (rosaxny). #32392
* worker:
  * Added support for passing a `transferList` along with `workerData`
    to the `Worker` constructor (Juan José Arboleda).
    #32278

New core collaborators:
With this release, we welcome three new Node.js core collaborators:
* himself65. #32734
* flarna (Gerhard Stoebich). #32620
* mildsunrise (Alba Mendez). #32525

PR-URL: #32813
@himself65

himself65 commented Apr 15, 2020

Copy link
Copy Markdown
Member

just tip. downstream #32689 or #32860
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@gabrielschulhof gabrielschulhof mentioned this pull request Apr 15, 2020
n-api: detect deadlocks in thread-safe function (take two) #32860
Closed
2 tasks done
@targos targos added dont-land-on-v10.x labels Apr 25, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jasnell jasnell jasnell approved these changes
@addaleax addaleax addaleax approved these changes
@cjihrig cjihrig cjihrig approved these changes
@legendecas legendecas legendecas approved these changes
+2 more reviewers
@devnexen devnexen devnexen approved these changes
@mhdawson mhdawson mhdawson approved these changes
Reviewers whose approvals may not affect merge requirements

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

c++ Issues and PRs that require attention from people who are familiar with C++. node-api Issues and PRs related to Node-API. semver-minor PRs that contain new features and should be released in the next minor version.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

napi_call_threadsafe_function should error if called from the main thread

Uh oh!

There was an error while loading. Please reload this page.

10 participants

@gabrielschulhof @nodejs-github-bot @himself65 @jasnell @addaleax @cjihrig @devnexen @legendecas @mhdawson @targos
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.