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.

Sign up for GitHub

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

Jump to bottom

src: deprecate two- and one-argument AtExit() #30227

Closed
wants to merge 1 commit into from
Closed

src: deprecate two- and one-argument AtExit() #30227

wants to merge 1 commit into from

Conversation

addaleax
Copy link
Member

@addaleax addaleax commented Nov 2, 2019

Using AtExit() without an Environment* pointer or providing
an argument is almost always a sign of improperly relying on global
state and/or using AtExit() as an addon when the addon-targeting
AddEnvironmentCleanupHook() would be the better choice.

Deprecate those variants. This also updates the addon docs to
refer to AddEnvironmentCleanupHook() rather than AtExit().

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

@nodejs-github-bot nodejs-github-bot added the c++ Issues and PRs that require attention from people who are familiar with C++. label Nov 2, 2019
@addaleax addaleax added embedding Issues and PRs related to embedding Node.js in another project. semver-minor PRs that contain new features and should be released in the next minor version. labels Nov 2, 2019
fhinkel
fhinkel reviewed Nov 3, 2019
View reviewed changes
test/addons/at-exit/binding.cc
@@ -3,7 +3,11 @@
#include <node.h>
#include <v8.h>

// TODO(addaleax): Maybe merge this file with the cctest for AtExit()?
Copy link
Member

Choose a reason for hiding this comment

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

Do you want to keep this as a todo or should we open an issue instead?

Copy link
Member Author

Choose a reason for hiding this comment

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

I can open an issue about it once this lands. 👍

@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/26306/

@addaleax addaleax added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Nov 4, 2019
@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/26314/

@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/26324/

@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/26352/

@addaleax
src: deprecate two- and one-argument AtExit()
7c59832 
Using `AtExit()` without an `Environment*` pointer or providing
an argument is almost always a sign of improperly relying on global
state and/or using `AtExit()` as an addon when the addon-targeting
`AddEnvironmentCleanupHook()` would be the better choice.

Deprecate those variants. This also updates the addon docs to
refer to `AddEnvironmentCleanupHook()` rather than `AtExit()`.
@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/26357/

@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/26371/

addaleax added a commit that referenced this pull request Nov 5, 2019
@addaleax
src: deprecate two- and one-argument AtExit()
5b2067c 
Using `AtExit()` without an `Environment*` pointer or providing
an argument is almost always a sign of improperly relying on global
state and/or using `AtExit()` as an addon when the addon-targeting
`AddEnvironmentCleanupHook()` would be the better choice.

Deprecate those variants. This also updates the addon docs to
refer to `AddEnvironmentCleanupHook()` rather than `AtExit()`.

PR-URL: #30227
Reviewed-By: David Carlier <devnexen@gmail.com>
Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com>
Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@addaleax
Copy link
Member Author

addaleax commented Nov 5, 2019

Landed in 5b2067c, thanks for the reviews!

@addaleax addaleax closed this Nov 5, 2019
@addaleax addaleax deleted the atexit-deprecate branch November 5, 2019 23:03
addaleax added a commit to addaleax/node that referenced this pull request Nov 5, 2019
@addaleax
test: remove AtExit() addon test
adccf30 
Move the one bit of the addon test that was not covered by the
cctest into the latter and delete the former.

Refs: nodejs#30227 (comment)
@addaleax addaleax mentioned this pull request Nov 5, 2019
Closed
3 tasks
Trott pushed a commit that referenced this pull request Nov 8, 2019
@addaleax @Trott
test: remove AtExit() addon test
55f4db1 
Move the one bit of the addon test that was not covered by the
cctest into the latter and delete the former.

Refs: #30227 (comment)

PR-URL: #30275
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
MylesBorins pushed a commit that referenced this pull request Nov 17, 2019
@addaleax @MylesBorins
src: deprecate two- and one-argument AtExit()
cd6d621 
Using `AtExit()` without an `Environment*` pointer or providing
an argument is almost always a sign of improperly relying on global
state and/or using `AtExit()` as an addon when the addon-targeting
`AddEnvironmentCleanupHook()` would be the better choice.

Deprecate those variants. This also updates the addon docs to
refer to `AddEnvironmentCleanupHook()` rather than `AtExit()`.

PR-URL: #30227
Reviewed-By: David Carlier <devnexen@gmail.com>
Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com>
Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
MylesBorins pushed a commit that referenced this pull request Nov 17, 2019
@addaleax @MylesBorins
test: remove AtExit() addon test
ab5bca3 
Move the one bit of the addon test that was not covered by the
cctest into the latter and delete the former.

Refs: #30227 (comment)

PR-URL: #30275
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
@BridgeAR BridgeAR mentioned this pull request Nov 19, 2019
Merged
MylesBorins added a commit to BridgeAR/node that referenced this pull request Nov 21, 2019
@MylesBorins
2019-11-21, Version 13.2.0 (Current)
8eda60c 
Notable changes:

* addons:
  * Deprecate one- and two-argument `AtExit()`. Use the three-argument
    variant of `AtExit()` or `AddEnvironmentCleanupHook()` instead
    (Anna Henningsen) nodejs#30227
* child_process,cluster:
  * The `serialization` option is added that allows child process
    IPC to use the V8 serialization API (to e.g., pass through data
    types like sets or maps) (Anna Henningsen)
    nodejs#30162
* deps:
  * Update V8 to 7.9
  * Update `npm` to 6.13.0 (Ruy Adorno)
    nodejs#30271
* embedder:
  * Exposes the ability to pass cli flags / options through an API
    as embedder (Shelley Vohr)
    nodejs#30466
  * Allow adding linked bindings to Environment (Anna Henningsen)
    nodejs#30274
* esm:
  * Unflag --experimental-modules (Guy Bedford)
    nodejs#29866
* stream:
  * Add `writable.writableCorked` property (Robert Nagy)
    nodejs#29012
* worker:
  * Allow specifying resource limits (Anna Henningsen)
    nodejs#26628
* v8:
  * The Serialization API is now stable (Anna Henningsen)
    nodejs#30234

PR-URL: nodejs#30547
MylesBorins added a commit that referenced this pull request Nov 21, 2019
@MylesBorins
2019-11-21, Version 13.2.0 (Current)
b8f8f05 
Notable changes:

* addons:
  * Deprecate one- and two-argument `AtExit()`. Use the three-argument
    variant of `AtExit()` or `AddEnvironmentCleanupHook()` instead
    (Anna Henningsen) #30227
* child_process,cluster:
  * The `serialization` option is added that allows child process
    IPC to use the V8 serialization API (to e.g., pass through data
    types like sets or maps) (Anna Henningsen)
    #30162
* deps:
  * Update V8 to 7.9
  * Update `npm` to 6.13.0 (Ruy Adorno)
    #30271
* embedder:
  * Exposes the ability to pass cli flags / options through an API
    as embedder (Shelley Vohr)
    #30466
  * Allow adding linked bindings to Environment (Anna Henningsen)
    #30274
* esm:
  * Unflag --experimental-modules (Guy Bedford)
    #29866
* stream:
  * Add `writable.writableCorked` property (Robert Nagy)
    #29012
* worker:
  * Allow specifying resource limits (Anna Henningsen)
    #26628
* v8:
  * The Serialization API is now stable (Anna Henningsen)
    #30234

PR-URL: #30547
MylesBorins pushed a commit that referenced this pull request Jan 12, 2020
@addaleax @MylesBorins
src: deprecate two- and one-argument AtExit()
0beb8a6 
Using `AtExit()` without an `Environment*` pointer or providing
an argument is almost always a sign of improperly relying on global
state and/or using `AtExit()` as an addon when the addon-targeting
`AddEnvironmentCleanupHook()` would be the better choice.

Deprecate those variants. This also updates the addon docs to
refer to `AddEnvironmentCleanupHook()` rather than `AtExit()`.

PR-URL: #30227
Reviewed-By: David Carlier <devnexen@gmail.com>
Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com>
Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
MylesBorins pushed a commit that referenced this pull request Jan 12, 2020
@addaleax @MylesBorins
test: remove AtExit() addon test
de719fc 
Move the one bit of the addon test that was not covered by the
cctest into the latter and delete the former.

Refs: #30227 (comment)

PR-URL: #30275
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
@targos targos mentioned this pull request Jan 15, 2020
Closed
BethGriggs pushed a commit that referenced this pull request Feb 6, 2020
@addaleax @BethGriggs
src: deprecate two- and one-argument AtExit()
60ac18c 
Using `AtExit()` without an `Environment*` pointer or providing
an argument is almost always a sign of improperly relying on global
state and/or using `AtExit()` as an addon when the addon-targeting
`AddEnvironmentCleanupHook()` would be the better choice.

Deprecate those variants. This also updates the addon docs to
refer to `AddEnvironmentCleanupHook()` rather than `AtExit()`.

PR-URL: #30227
Reviewed-By: David Carlier <devnexen@gmail.com>
Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com>
Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
BethGriggs pushed a commit that referenced this pull request Feb 6, 2020
@addaleax @BethGriggs
test: remove AtExit() addon test
71a3f48 
Move the one bit of the addon test that was not covered by the
cctest into the latter and delete the former.

Refs: #30227 (comment)

PR-URL: #30275
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
@MylesBorins MylesBorins mentioned this pull request Feb 8, 2020
Merged
@nick-erm nick-erm mentioned this pull request May 7, 2020
Open
@ewanharris ewanharris mentioned this pull request Apr 21, 2021
Merged
@frederickjansen frederickjansen mentioned this pull request Nov 3, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@devnexen devnexen devnexen approved these changes

@jasnell jasnell jasnell approved these changes

@danbev danbev danbev approved these changes

@fhinkel fhinkel fhinkel approved these changes

Assignees
No one assigned
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. c++ Issues and PRs that require attention from people who are familiar with C++. embedding Issues and PRs related to embedding Node.js in another project. 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.
6 participants
@addaleax @nodejs-github-bot @fhinkel @danbev @jasnell @devnexen