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

New dlopen flags API #12848

Closed
ezequielgarcia opened this issue May 5, 2017 · 7 comments
Closed

New dlopen flags API #12848

ezequielgarcia opened this issue May 5, 2017 · 7 comments
Labels
addons Issues and PRs related to native addons. feature request Issues that request new features to be added to Node.js.

Comments

@ezequielgarcia
Copy link
Contributor

Let's discuss how an API for dlopen flags should look like. A proposal has been made on #12794, where it was clear that once we sort out the API, the implementation is fairly straightforward.

As @jasnell and @sam-github pointed out it would be ideal to be able to pass these flags to act locally to a specific addon. On the other hand, this might require more work.

Setting global dlopen flags seems easier, at the cost of being potentially more fragile. Indeed, it would pollute the namespace, breaking if two addons load collisioning symbols. On the other side, this is highly unlikely. In favor of this, note that the global solution is what Python seems to do.

So, I propose two APIs:

(1) Global, using process module. Similar to #12794.

process.setdlopenFlags(os.constants.dlopen.RTLD_NOW | os.constants.dlopen.RTLD_GLOBAL);
var foo = require('foo');
process.setdlopenFlags(0);

(2) Also global (?), putting the setter in the require object:

require.setdlopenFlags(os.constants.dlopen.RTLD_NOW | os.constants.dlopen.RTLD_GLOBAL);
var foo = require('foo');
require.setdlopenFlags(0);

IMO, in this last case, the semantics are misleading. When the require.setdlopenFlags() call is made, the current module has already been loaded. So the dlopen flags are set for a JS script that is already loaded. The Module instance for the addon has to check its parent dlopen flags.

(3) Local to an addon, passing an object as an extra argument to require().

var os = require('os');
var foo = require('foo', {dlopenFlags: os.constants.dlopen.RTLD_NOW | os.constants.dlopen.RTLD_GLOBAL})

As far as I can see, passing an argument to require is the only way of keeping the flag local to a module. The addon to-be-loaded must first create a Module instance, which is created in the context of the require call.

@jasnell
Copy link
Member

jasnell commented May 5, 2017

Every module gets it's own copy of the require() function so in that sense it's not global. We can hang the API off require.setdlopenFlags(...) and have it be specific to just that one instance of require() (and any native modules loaded via that instance of require(). There are some changes that would need to be made at the binding layer, but those shouldn't be too complicated.

@mscdex mscdex added addons Issues and PRs related to native addons. feature request Issues that request new features to be added to Node.js. labels May 5, 2017
@addaleax
Copy link
Member

addaleax commented May 5, 2017

@jasnell That’s not so easy, though – often enough, addons are required in a nested fashion (or through some more complex logic, like what the bindings module does)…

I think I’m good with option 1), because the dlopen flags actually can affect the process in a non-local fashion.

@ezequielgarcia
Copy link
Contributor Author

I understand that changing the require as in (3) is syntactically invasive -- But OTOH, isn't this expressing the intent most clearly? Note that it also requires changes to the bindings module.

Option (1) is really the simplest one.

@gireeshpunathil
Copy link
Member

I guess with (1) and (3) the tradeoff is between development effort vs. reliability and serviceability. As there is no imminent impact to the released versions and this is a potential feature, I am in favor of (3) - as it is a clean solution, and cause least side effects.

@jasnell
Copy link
Member

jasnell commented May 7, 2017

@addaleax ... oh yeah, I definitely understand the complexities there. I've had a background thread running in the back of my brain trying to figure out the best way to isolate this. One possible (and likely quite bad) option would be to simply allow for a new options parameter on require() .... (go ahead and start throwing things at me ;-) ...)

e.g. require('the.module.node', { dlopen: '...' })

If we limited support for this such an options object specifically to loading native addons, then we could potentially get away with it, but even then, the issue that you raise is still potentially there.

I dunno, I'm just going to need to stew on this one a bit more.

@ezequielgarcia
Copy link
Contributor Author

@jasnell @addaleax @mscdex @gireeshpunathil I've updated #12794, exploring the require(..., opts) route.

@bnoordhuis
Copy link
Member

#12794 was merged last month. I'll close this out.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
addons Issues and PRs related to native addons. feature request Issues that request new features to be added to Node.js.
Projects
None yet
Development

No branches or pull requests

6 participants