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

src: fix linking when built as shared library #3098

Closed
wants to merge 1 commit into from
Closed

src: fix linking when built as shared library #3098

wants to merge 1 commit into from

Conversation

zcbenz
Copy link
Contributor

@zcbenz zcbenz commented Sep 28, 2015

This is another solution to fix the problem in #3046 as suggested by @bnoordhuis.

Currently linking Node as shared library in third party embedders will result in undefined symbols problem because the constructor of Environment is inlined and is calling constructor of debugger::Agent, which is not exported. This fix moves the constructor and destructor of Environment to src/env.cc to avoid leaking implementation details to solve this problem.

Following changes are also made for the fix:

  1. The types of Environment's properties are changed from v8::Persistent to v8::Global, so we don't have to manually reset all of them, this is because the ENVIRONMENT_STRONG_PERSISTENT_PROPERTIES macro is removed in env-inl.h and calling Reset() in env.cc would be very hard.
  2. A new IsolateData::Scope class is introduced to manage IsolateData automatically instead of manually calling Put(), so we can rely the destructor to do clean up job.
  3. The *_buffer_ members are managed by std::vector so we don't have to manually delete them in destructor.
  4. PersistentToLocal are extended to handle v8::Global.
  5. The Environment::New and Environment::Dispose are now exported functions so embedders can still use them as APIs without knowing the definition of Environment's constructor.

@brendanashworth brendanashworth added the c++ Issues and PRs that require attention from people who are familiar with C++. label Sep 28, 2015
@ChALkeR ChALkeR added the build Issues and PRs related to build files or the CI. label Sep 28, 2015
@@ -25,7 +34,6 @@ inline Environment::IsolateData* Environment::IsolateData::GetOrCreate(
isolate_data = new IsolateData(isolate, loop);
isolate->SetData(kIsolateSlot, isolate_data);
}
isolate_data->ref_count_ += 1;
Copy link
Member

Choose a reason for hiding this comment

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

This makes GetOrCreate() rather dangerous to use. Can I suggest that you remove Get(), GetOrCreate() and Put() and do all management inside the Scope constructor and destructor, with a method or dereference operator to get the raw IsolateData*?

@bnoordhuis
Copy link
Member

The types of Environment's properties are changed from v8::Persistent to v8::Global, so we don't have to manually reset all of them

I don't really like how that makes the handles movable when they're noncopyable now. I'd be okay with not undef'ing ENVIRONMENT_STRONG_PERSISTENT_PROPERTIES in env.h.

The Environment::New and Environment::Dispose are now exported functions so embedders can still use them as APIs without knowing the definition of Environment's constructor.

I'm not really a fan of exposing what are essentially implementation details. Can other collaborators chime in?

@zcbenz
Copy link
Contributor Author

zcbenz commented Sep 28, 2015

I don't really like how that makes the handles movable when they're noncopyable now. I'd be okay with not undef'ing ENVIRONMENT_STRONG_PERSISTENT_PROPERTIES in env.h.

If we are reserving the ENVIRONMENT_STRONG_PERSISTENT_PROPERTIES in env.h, should I revert the other changes like the IsolateData::Scope and std::vector ones? Since the purpose of them are to make sure the sequence of destruction is not changed.

I'm not really a fan of exposing what are essentially implementation details

I'm not sure if Environment::New belongs to implementation details, in Electron we used it to create an empty Environment under some conditions.

@bnoordhuis
Copy link
Member

If we are reserving the ENVIRONMENT_STRONG_PERSISTENT_PROPERTIES in env.h, should I revert the other changes like the IsolateData::Scope and std::vector ones?

I'll leave that up to you. I think the std::vector change is an improvement and IsolateData::Sope will be as well with the suggested changes. Maybe break it in two commits, one removing the #undef, the other with code improvements.

I'm not sure if Environment::New belongs to implementation details, in Electron we used it to create an empty Environment under some conditions.

I don't consider it part of the public API, that's what CreateEnvironment() is for.

@zcbenz
Copy link
Contributor Author

zcbenz commented Sep 28, 2015

I'll leave that up to you. I think the std::vector change is an improvement and IsolateData::Sope will be as well with the suggested changes. Maybe break it in two commits, one removing the #undef, the other with code improvements.

Cool, I'll split it into two commits.

I don't consider it part of the public API, that's what CreateEnvironment() is for.

So should we only expose the Environment::Dispose? That seems to be only API to destroy an Environment.

@bnoordhuis
Copy link
Member

So should we only expose the Environment::Dispose? That seems to be only API to destroy an Environment.

I guess for consistency it's better to add a FreeEnvironment() to src/node.h.

@zcbenz
Copy link
Contributor Author

zcbenz commented Sep 28, 2015

I have updated the commit to simply hide Environment's constructor and destruct, and add a node:: FreeEnvironment API.

@bnoordhuis
Copy link
Member

Circling back to the original link error message you reported in #3046, now that there is a way to create and dispose Environments through opaque pointers, do the constructor and destructor still need to be moved around?

Since debugger::Agent's interface is not exported, third party embedders
will have linking errors if they call Environment's destructor directly.
@zcbenz
Copy link
Contributor Author

zcbenz commented Sep 28, 2015

You are right, having an node::FreeEnvironment API should be enough, I have updated the commit.

@bnoordhuis
Copy link
Member

LGTM, thanks. CI: https://ci.nodejs.org/job/node-test-pull-request/393/

@bnoordhuis
Copy link
Member

Forgot about this, sorry about that. Another CI run: https://ci.nodejs.org/job/node-test-pull-request/695/

@jasnell jasnell added the stalled Issues and PRs that are stalled. label Mar 22, 2016
@estliberitas estliberitas force-pushed the master branch 2 times, most recently from 7da4fd4 to c7066fb Compare April 26, 2016 05:22
@eljefedelrodeodeljefe
Copy link
Contributor

@bnoordhuis is there anything against merging this?

bnoordhuis pushed a commit that referenced this pull request May 19, 2016
Since debugger::Agent's interface is not exported, third party embedders
will have linking errors if they call Environment's destructor directly.

PR-URL: #3098
Reviewed-By: Ben Noordhuis <[email protected]>
@bnoordhuis
Copy link
Member

Nothing, I merely forgot about it again. Landed in 2ab75b7 and I'll update the tags; it can land in a semver-minor release if desired.

@bnoordhuis bnoordhuis closed this May 19, 2016
@bnoordhuis bnoordhuis added semver-minor PRs that contain new features and should be released in the next minor version. lts-watch-v4.x and removed stalled Issues and PRs that are stalled. labels May 19, 2016
@eljefedelrodeodeljefe
Copy link
Contributor

Awesome thanks. Making a reference to nodejs/build#359... there are several guys having done work on this in forks and external repos. I'll collect some info and hope to drive android support forward,

Fishrock123 pushed a commit that referenced this pull request Jun 27, 2016
Since debugger::Agent's interface is not exported, third party embedders
will have linking errors if they call Environment's destructor directly.

PR-URL: #3098
Reviewed-By: Ben Noordhuis <[email protected]>
@Fishrock123 Fishrock123 mentioned this pull request Jun 27, 2016
@MylesBorins
Copy link
Contributor

@nodejs/lts would we like to include this in v4.5.0?

@Fishrock123
Copy link
Contributor

@thealphanerd might not be a bad idea, +0.5

Fishrock123 pushed a commit that referenced this pull request Jul 5, 2016
Since debugger::Agent's interface is not exported, third party embedders
will have linking errors if they call Environment's destructor directly.

PR-URL: #3098
Reviewed-By: Ben Noordhuis <[email protected]>
@Fishrock123 Fishrock123 mentioned this pull request Jul 5, 2016
Fishrock123 added a commit that referenced this pull request Jul 6, 2016
Notable changes:

* buffer: Added `buffer.swap64()` to compliment `swap16()` &
`swap32()`. (Zach Bjornson) #7157
* build: New `configure` options have been added for building Node.js
as a shared library. (Stefan Budeanu)
#6994
  - The options are: `--shared`, `--without-v8-platform` &
`--without-bundled-v8`.
* crypto: Root certificates have been updated. (Ben Noordhuis)
#7363
* debugger: The server address is now configurable via
`--debug=<address>:<port>`. (Ben Noordhuis)
#3316
* npm: Upgraded npm to v3.10.3 (Kat Marchán)
#7515 & (Rebecca Turner)
#7410
* readline: Added the `prompt` option to the readline constructor.
(Evan Lucas) #7125
* repl / vm: `sigint`/`ctrl+c` will now break out of infinite loops
without stopping the Node.js instance. (Anna Henningsen)
#6635
* src:
  - Added a `node::FreeEnvironment` public C++ API. (Cheng Zhao)
#3098
  - Refactored `require('constants')`, constants are now available
directly from their respective modules. (James M Snell)
#6534
* stream: Improved `readable.read()` performance by up to 70%. (Brian
White) #7077
* timers: `setImmediate()` is now up to 150% faster in some situations.
(Andras) #6436
* util: Added a `breakLength` option to `util.inspect()` to control how
objects are formatted across lines. (cjihrig)
#7499
* v8-inspector: Experimental support has been added for debugging
Node.js over the inspector protocol. (Ali Ijaz Sheikh)
#6792
  - *Note: This feature is experimental, and it could be altered or
removed.*
  - You can try this feature by running Node.js with the `--inspect`
flag.

Refs: #7441
PR-URL: #7550
Fishrock123 added a commit that referenced this pull request Jul 6, 2016
Notable changes:

* buffer: Added `buffer.swap64()` to compliment `swap16()` &
`swap32()`. (Zach Bjornson) #7157
* build: New `configure` options have been added for building Node.js
as a shared library. (Stefan Budeanu)
#6994
  - The options are: `--shared`, `--without-v8-platform` &
`--without-bundled-v8`.
* crypto: Root certificates have been updated. (Ben Noordhuis)
#7363
* debugger: The server address is now configurable via
`--debug=<address>:<port>`. (Ben Noordhuis)
#3316
* npm: Upgraded npm to v3.10.3 (Kat Marchán)
#7515 & (Rebecca Turner)
#7410
* readline: Added the `prompt` option to the readline constructor.
(Evan Lucas) #7125
* repl / vm: `sigint`/`ctrl+c` will now break out of infinite loops
without stopping the Node.js instance. (Anna Henningsen)
#6635
* src:
  - Added a `node::FreeEnvironment` public C++ API. (Cheng Zhao)
#3098
  - Refactored `require('constants')`, constants are now available
directly from their respective modules. (James M Snell)
#6534
* stream: Improved `readable.read()` performance by up to 70%. (Brian
White) #7077
* timers: `setImmediate()` is now up to 150% faster in some situations.
(Andras) #6436
* util: Added a `breakLength` option to `util.inspect()` to control how
objects are formatted across lines. (cjihrig)
#7499
* v8-inspector: Experimental support has been added for debugging
Node.js over the inspector protocol. (Ali Ijaz Sheikh)
#6792
  - *Note: This feature is experimental, and it could be altered or
removed.*
  - You can try this feature by running Node.js with the `--inspect`
flag.

Refs: #7441
PR-URL: #7550
Fishrock123 added a commit that referenced this pull request Jul 6, 2016
Notable changes:

* buffer: Added `buffer.swap64()` to compliment `swap16()` &
`swap32()`. (Zach Bjornson) #7157
* build: New `configure` options have been added for building Node.js
as a shared library. (Stefan Budeanu)
#6994
  - The options are: `--shared`, `--without-v8-platform` &
`--without-bundled-v8`.
* crypto: Root certificates have been updated. (Ben Noordhuis)
#7363
* debugger: The server address is now configurable via
`--debug=<address>:<port>`. (Ben Noordhuis)
#3316
* npm: Upgraded npm to v3.10.3 (Kat Marchán)
#7515 & (Rebecca Turner)
#7410
* readline: Added the `prompt` option to the readline constructor.
(Evan Lucas) #7125
* repl / vm: `sigint`/`ctrl+c` will now break out of infinite loops
without stopping the Node.js instance. (Anna Henningsen)
#6635
* src:
  - Added a `node::FreeEnvironment` public C++ API. (Cheng Zhao)
#3098
  - Refactored `require('constants')`, constants are now available
directly from their respective modules. (James M Snell)
#6534
* stream: Improved `readable.read()` performance by up to 70%. (Brian
White) #7077
* timers: `setImmediate()` is now up to 150% faster in some situations.
(Andras) #6436
* util: Added a `breakLength` option to `util.inspect()` to control how
objects are formatted across lines. (cjihrig)
#7499
* v8-inspector: Experimental support has been added for debugging
Node.js over the inspector protocol. (Ali Ijaz Sheikh)
#6792
  - *Note: This feature is experimental, and it could be altered or
removed.*
  - You can try this feature by running Node.js with the `--inspect`
flag.

Refs: #7441
PR-URL: #7550
lukesampson pushed a commit to ScoopInstaller/Scoop that referenced this pull request Jul 7, 2016
### Notable changes

* **buffer**: Added `buffer.swap64()` to compliment `swap16()` & `swap32()`. (Zach Bjornson) [#7157](nodejs/node#7157)
* **build**: New `configure` options have been added for building Node.js as a shared library. (Stefan Budeanu) [#6994](nodejs/node#6994)
  - The options are: `--shared`, `--without-v8-platform` & `--without-bundled-v8`.
* **crypto**: Root certificates have been updated. (Ben Noordhuis) [#7363](nodejs/node#7363)
* **debugger**: The server address is now configurable via `--debug=<address>:<port>`. (Ben Noordhuis) [#3316](nodejs/node#3316)
* **npm**: Upgraded npm to v3.10.3 (Kat Marchán) [#7515](nodejs/node#7515) & (Rebecca Turner) [#7410](nodejs/node#7410)
* **readline**: Added the `prompt` option to the readline constructor. (Evan Lucas) [#7125](nodejs/node#7125)
* **repl / vm**: `sigint`/`ctrl+c` will now break out of infinite loops without stopping the Node.js instance. (Anna Henningsen) [#6635](nodejs/node#6635)
* **src**:
  - Added a `node::FreeEnvironment` public C++ API. (Cheng Zhao) [#3098](nodejs/node#3098)
  - Refactored `require('constants')`, constants are now available directly from their respective modules. (James M Snell) [#6534](nodejs/node#6534)
* **stream**: Improved `readable.read()` performance by up to 70%. (Brian White) [#7077](nodejs/node#7077)
* **timers**: `setImmediate()` is now up to 150% faster in some situations. (Andras) [#6436](nodejs/node#6436)
* **util**: Added a `breakLength` option to `util.inspect()` to control how objects are formatted across lines. (cjihrig) [#7499](nodejs/node#7499)
* **v8-inspector**: Experimental support has been added for debugging Node.js over the inspector protocol. (Ali Ijaz Sheikh) [#6792](nodejs/node#6792)
  - **Note: This feature is _experimental_, and it could be altered or removed.**
  - You can try this feature by running Node.js with the `--inspect` flag.
MylesBorins pushed a commit that referenced this pull request Jul 11, 2016
Since debugger::Agent's interface is not exported, third party embedders
will have linking errors if they call Environment's destructor directly.

PR-URL: #3098
Reviewed-By: Ben Noordhuis <[email protected]>
MylesBorins pushed a commit that referenced this pull request Jul 11, 2016
Since debugger::Agent's interface is not exported, third party embedders
will have linking errors if they call Environment's destructor directly.

PR-URL: #3098
Reviewed-By: Ben Noordhuis <[email protected]>
MylesBorins pushed a commit that referenced this pull request Jul 12, 2016
Since debugger::Agent's interface is not exported, third party embedders
will have linking errors if they call Environment's destructor directly.

PR-URL: #3098
Reviewed-By: Ben Noordhuis <[email protected]>
MylesBorins pushed a commit that referenced this pull request Jul 12, 2016
Notable Changes:

This list is not yet complete. Please comment in the thread with
commits you think should be included. Descriptions will also be
updated in a later release candidate.

Semver Minor:

* buffer:
 * backport new buffer constructor APIs to v4.x
   (Сковорода Никита Андреевич)
   #7562
 * ignore negative allocation lengths (Anna Henningsen)
   #7562
 * backport --zero-fill-buffers cli option (James M Snell)
   #5745
* build:
  * add Intel Vtune profiling support (Chunyang Dai)
    #5527
* repl:
  * copying tabs shouldn't trigger completion (Eugene Obrezkov)
    #5958
* src:
  * add node::FreeEnvironment public API (Cheng Zhao)
    #3098
* test:
  * run v8 tests from node tree (Bryon Leung)
    #4704
* V8:
  * backport 9c927d0f01 from V8 upstream (Myles Borins)
    #7451
  * cherry-pick 68e89fb from v8's upstream (Fedor Indutny)
    #3779

Semver Patch:

* **libuv**:
  * upgrade libuv to 1.9.1 (Saúl Ibarra Corretgé)
    #6796
  * upgrade libuv to 1.9.0 (Saúl Ibarra Corretgé)
    #5994
@MylesBorins MylesBorins mentioned this pull request Jul 12, 2016
MylesBorins pushed a commit that referenced this pull request Jul 13, 2016
Notable Changes:

This list is not yet complete. Please comment in the thread with
commits you think should be included. Descriptions will also be
updated in a later release candidate.

Semver Minor:

* buffer:
 * backport new buffer constructor APIs to v4.x
   (Сковорода Никита Андреевич)
   #7562
 * ignore negative allocation lengths (Anna Henningsen)
   #7562
 * backport --zero-fill-buffers cli option (James M Snell)
   #5745
* build:
  * add Intel Vtune profiling support (Chunyang Dai)
    #5527
* repl:
  * copying tabs shouldn't trigger completion (Eugene Obrezkov)
    #5958
* src:
  * add node::FreeEnvironment public API (Cheng Zhao)
    #3098
* test:
  * run v8 tests from node tree (Bryon Leung)
    #4704
* V8:
  * Add post portem data to imrpove object inspection and function's
    context variables inspection (Fedor Indutny)
    #3779

Semver Patch:

* **libuv**:
  * upgrade libuv to 1.9.1 (Saúl Ibarra Corretgé)
    #6796
  * upgrade libuv to 1.9.0 (Saúl Ibarra Corretgé)
    #5994
MylesBorins pushed a commit that referenced this pull request Jul 14, 2016
Since debugger::Agent's interface is not exported, third party embedders
will have linking errors if they call Environment's destructor directly.

PR-URL: #3098
Reviewed-By: Ben Noordhuis <[email protected]>
MylesBorins pushed a commit that referenced this pull request Jul 14, 2016
Since debugger::Agent's interface is not exported, third party embedders
will have linking errors if they call Environment's destructor directly.

PR-URL: #3098
Reviewed-By: Ben Noordhuis <[email protected]>
MylesBorins pushed a commit that referenced this pull request Jul 14, 2016
Notable Changes:

This list is not yet complete. Please comment in the thread with
commits you think should be included. Descriptions will also be
updated in a later release candidate.

Semver Minor:

* buffer:
 * backport new buffer constructor APIs to v4.x
   (Сковорода Никита Андреевич)
   #7562
 * backport --zero-fill-buffers cli option (James M Snell)
   #5745
* build:
  * add Intel Vtune profiling support (Chunyang Dai)
    #5527
* repl:
  * copying tabs shouldn't trigger completion (Eugene Obrezkov)
    #5958
* src:
  * add node::FreeEnvironment public API (Cheng Zhao)
    #3098
* test:
  * run v8 tests from node tree (Bryon Leung)
    #4704
* V8:
  * Add post portem data to imrpove object inspection and function's
    context variables inspection (Fedor Indutny)
    #3779

Semver Patch:

* **buffer**:
  * ignore negative allocation lengths (Anna Henningsen)
    #7562
* **libuv**:
  * upgrade libuv to 1.9.1 (Saúl Ibarra Corretgé)
    #6796
  * upgrade libuv to 1.9.0 (Saúl Ibarra Corretgé)
    #5994
* **npm**:
  * upgrade to 2.15.9 (Kat Marchán)
    #7692
MylesBorins pushed a commit that referenced this pull request Aug 15, 2016
Notable Changes:

Semver Minor:

* buffer:
 * backport new buffer constructor APIs to v4.x
   (Сковорода Никита Андреевич)
   #7562
 * backport --zero-fill-buffers cli option (James M Snell)
   #5745
* build:
  * add Intel Vtune profiling support (Chunyang Dai)
    #5527
* repl:
  * copying tabs shouldn't trigger completion (Eugene Obrezkov)
    #5958
* src:
  * add node::FreeEnvironment public API (Cheng Zhao)
    #3098
* test:
  * run v8 tests from node tree (Bryon Leung)
    #4704
* V8:
  * Add post mortem data to improve object inspection and function's
    context variables inspection (Fedor Indutny)
    #3779

Semver Patch:

* **buffer**:
  * ignore negative allocation lengths (Anna Henningsen)
    #7562
* **crypto**:
  * update root certificates (Ben Noordhuis)
    #7363
* **libuv**:
  * upgrade libuv to 1.9.1 (Saúl Ibarra Corretgé)
    #6796
  * upgrade libuv to 1.9.0 (Saúl Ibarra Corretgé)
    #5994
* **npm**:
  * upgrade to 2.15.9 (Kat Marchán)
    #7692
MylesBorins pushed a commit that referenced this pull request Aug 16, 2016
Notable Changes:

Semver Minor:

* buffer:
 * backport new buffer constructor APIs to v4.x
   (Сковорода Никита Андреевич)
   #7562
 * backport --zero-fill-buffers cli option (James M Snell)
   #5745
* build:
  * add Intel Vtune profiling support (Chunyang Dai)
    #5527
* repl:
  * copying tabs shouldn't trigger completion (Eugene Obrezkov)
    #5958
* src:
  * add node::FreeEnvironment public API (Cheng Zhao)
    #3098
* test:
  * run v8 tests from node tree (Bryon Leung)
    #4704
* V8:
  * Add post mortem data to improve object inspection and function's
    context variables inspection (Fedor Indutny)
    #3779

Semver Patch:

* **buffer**:
  * ignore negative allocation lengths (Anna Henningsen)
    #7562
* **crypto**:
  * update root certificates (Ben Noordhuis)
    #7363
* **libuv**:
  * upgrade libuv to 1.9.1 (Saúl Ibarra Corretgé)
    #6796
  * upgrade libuv to 1.9.0 (Saúl Ibarra Corretgé)
    #5994
* **npm**:
  * upgrade to 2.15.9 (Kat Marchán)
    #7692
MylesBorins pushed a commit that referenced this pull request Aug 16, 2016
Notable Changes:

Semver Minor:

* buffer:
 * backport new buffer constructor APIs to v4.x
   (Сковорода Никита Андреевич)
   #7562
 * backport --zero-fill-buffers cli option (James M Snell)
   #5745
* build:
  * add Intel Vtune profiling support (Chunyang Dai)
    #5527
* repl:
  * copying tabs shouldn't trigger completion (Eugene Obrezkov)
    #5958
* src:
  * add node::FreeEnvironment public API (Cheng Zhao)
    #3098
* test:
  * run v8 tests from node tree (Bryon Leung)
    #4704
* V8:
  * Add post mortem data to improve object inspection and function's
    context variables inspection (Fedor Indutny)
    #3779

Semver Patch:

* **buffer**:
  * ignore negative allocation lengths (Anna Henningsen)
    #7562
* **crypto**:
  * update root certificates (Ben Noordhuis)
    #7363
* **libuv**:
  * upgrade libuv to 1.9.1 (Saúl Ibarra Corretgé)
    #6796
  * upgrade libuv to 1.9.0 (Saúl Ibarra Corretgé)
    #5994
* **npm**:
  * upgrade to 2.15.9 (Kat Marchán)
    #7692
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
build Issues and PRs related to build files or the CI. c++ Issues and PRs that require attention from people who are familiar with C++. semver-minor PRs that contain new features and should be released in the next minor version.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants