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: kickstart addon by calling its constructor #20114

Closed

Conversation

gabrielschulhof
Copy link
Contributor

If the addon does not self-register and a well-known Init symbol cannot
be found, look for a version-agnostic symbol which calls the library
constructor and call it if found.

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 c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs related to general changes in the lib or src directory. labels Apr 17, 2018
@gabrielschulhof
Copy link
Contributor Author

I'll add the documentation if this modification is accepted.

@Trott Trott added the semver-minor PRs that contain new features and should be released in the next minor version. label Apr 17, 2018
@Trott
Copy link
Member

Trott commented Apr 17, 2018

@nodejs/addon-api

@bnoordhuis
Copy link
Member

Couldn't you simply emit multiple node_register_module_vXX symbols instead of devising a third add-on loading mechanism?

I assume the goal is to be automatically forward-compatible but that's not necessarily a good idea. For starters, it cements the layout of struct node_module - that could never be changed again if this lands.

That said... that ship may have sailed already because nm_version == -1 is treated specially. Kinda frustrating #11975, #14902 and #19262 were reviewed by nine or ten people and no one called it out.

@gabrielschulhof
Copy link
Contributor Author

@bnoordhuis I don't understand why this particular PR would cement struct node_module. This merely simulates upon subsequent load what the library loader would do upon first load. The only thing this cements AFAICT is that we expect a newly loaded addon to call a function exposed by Node.js either in response to a "stimulus" by the library loader, or, with this change, in response to a "stimulus" by Node.js.

This PR makes no assumption about which Node.js function the addon calls, nor about the structure of the data it passes to that function.

For N-API purposes, I don't know that it would help that I scanned over a range of node_register_module_vXX symbols, because when you build the N-API module, you don't know ahead of time what values of XX to cover. Also, scanning over a range of symbols would only be useful if we could call any of them that we find. So, I guess we could do a bunch of special cases like

if (found-v-46) {
  // call it like this
} else if (found-v-47) {
  // call it some other way
} else if (found-v-63) {
  // call it yet another way
}

but since all these symbols take V8 objects, we can only call one such symbol, and only in such a way as is compatible with the current V8 ABI. So, scanning over a range of symbols would only make sense if we controlled the signature of those symbols.

I think the lowest common denominator, as with the library constructor, is void (*)(void);. The only missing piece is that dlopen() doesn't run the constructor again. So, come to think of it, the ability to save multiple versions of an addon into a single shared object is actually kind of orthogonal to this PR and useful in its own right.

@gabrielschulhof
Copy link
Contributor Author

Wait a sec - this short-circuits the search for the version-specific init special symbol. I need to fix that.

@gabrielschulhof
Copy link
Contributor Author

Like, what if the module self-registers, but with the wrong version? It should still look for the version-specific Init.

@bnoordhuis
Copy link
Member

I don't understand why this particular PR would cement struct node_module.

It links a struct node_module into the linked list. That means Node.js and the add-on need to agree on its layout because Node.js queries it later on.

Since there is no version number to sanity-check on when mp->nm_version == -1, any change to the struct breaks existing add-ons because they'll be using the old, incompatible layout.

@gabrielschulhof
Copy link
Contributor Author

@bnoordhuis this PR doesn't add the module to the linked list. It merely invokes the library constructor the second time around.

Can we not retain the old structure for the case where mp->nm_version == -1?

Whatever problems there are with the addon loading we will have whether or not this PR lands, and AFAICT this PR doesn't add to them.

If the addon does not self-register and a well-known Init symbol cannot
be found, look for a version-agnostic symbol which calls the library
constructor and call it if found.
@bnoordhuis
Copy link
Member

Can we not retain the old structure for the case where mp->nm_version == -1?

No, that's pinning us down for all eternity. Early Node.js releases were unversioned but we explicitly moved away from that because it's untenable in the long run.

this PR doesn't add to them

It adds a second API that exploits what is essentially an oversight. It'll make it twice as hard to fix in the future.

@gabrielschulhof
Copy link
Contributor Author

@bnoordhuis I've tried a different tack. Let's see where that leads us.

@gabrielschulhof gabrielschulhof deleted the kickstart-addon branch April 23, 2018 21:25
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++. lib / src Issues and PRs related to general changes in the lib or src directory. 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.

4 participants