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

bindings: adding process.binding('builtins') method #2180

Closed
wants to merge 2 commits into from
Closed

bindings: adding process.binding('builtins') method #2180

wants to merge 2 commits into from

Conversation

thlorenz
Copy link
Contributor

  • this method traverses the modlist_builtin linked list and returns an
    object which has binding names as keys and their versions as values

Sample Output:

➝  ./node
> process.binding('builtins')
{ tls_wrap: 44,
  crypto: 44,
  uv: 44,
  udp_wrap: 44,
  process_wrap: 44,
  tty_wrap: 44,
  timer_wrap: 44,
  tcp_wrap: 44,
  stream_wrap: 44,
  spawn_sync: 44,
  smalloc: 44,
  signal_wrap: 44,
  pipe_wrap: 44,
  zlib: 44,
  v8: 44,
  os: 44,
  http_parser: 44,
  fs: 44,
  contextify: 44,
  buffer: 44,
  js_stream: 44,
  cares_wrap: 44,
  fs_event_wrap: 44,
  async_wrap: 44 }

This is in a sense the binding counter part to process.binding('natives'), except for bindings and the fact that it doesn't include any sources.

- this method traverses the `modlist_builtin` linked list and returns an
  object which has binding names as keys and their versions as values
@brendanashworth brendanashworth added the c++ Issues and PRs that require attention from people who are familiar with C++. label Jul 14, 2015
@trevnorris
Copy link
Contributor

All of these will always have the same version. It might make more sense to do something like

{  version: 44,
   bindings: [<list>]
}

/cc @bnoordhuis
Am I correct in that assumption?

@thlorenz
Copy link
Contributor Author

I wasn't sure about the meaning of the versions, but I figured they could be different (why else would each binding have one).
I could change this to an array or include a bindings hash which adds different values, i.e. file_path.
Not sure if that information would be useful though.

If we return a bindings array we may also exclude the version info all together, i.e. just return an Array instead of an Object. Again I'm not sure of the importance/significance of the version field.

@trevnorris
Copy link
Contributor

@thlorenz The version is the same as defined as NODE_MODULE_VERSION in src/node_versions.h.

Since developers can get the module version as process.versions.modules I'd say just make an array and leave out other information.

@thlorenz
Copy link
Contributor Author

Sounds good, will update the PR tomorrow.

@vkurchatkin
Copy link
Contributor

@thlorenz may I ask, what is it for? process.binding is private so it doesn't expose any new functionality to the users

@thlorenz
Copy link
Contributor Author

@vkurchatkin this is just to get a list of builtin bindings in case you need to know that information.
However you can only read that list (of strings), but not access the bindings or their source.
So it doesn't expose any functionality per se.

@thlorenz
Copy link
Contributor Author

@trevnorris

I'd say just make an array and leave out other information.

Realized that the exports defined here is always an Local<Object>.
So trying to return a Local<Array> just for this special case seems inconsistent and would make that one extra code path very different from all other else if paths since we couldn't use this value return.

So for sake of consistency and simplicity I'll create an object instead with bindings property like you suggested before (except without the version as that is easily retrieved via process.versions.modules).
Returning an object has the added benefit that we could add more information via extra properties later without breaking backwards compatibility.

@thlorenz
Copy link
Contributor Author

Adapted the implementation to return the following:

➝  ./node
> process.binding('builtins')
{ bindings:
   [ 'tls_wrap',
     'crypto',
     'uv',
     'udp_wrap',
     'process_wrap',
     'tty_wrap',
     'timer_wrap',
     'tcp_wrap',
     'stream_wrap',
     'spawn_sync',
     'smalloc',
     'signal_wrap',
     'pipe_wrap',
     'zlib',
     'v8',
     'os',
     'http_parser',
     'fs',
     'contextify',
     'buffer',
     'js_stream',
     'cares_wrap',
     'fs_event_wrap',
     'async_wrap' ] }

@bnoordhuis
Copy link
Member

I don't see the point. process.binding() is internal, the list of bindings can and will change over time, and core doesn't have a need for it in the first place.

@thlorenz
Copy link
Contributor Author

@bnoordhuis

the list of bindings can and will change over time

That's exactly why it'd be useful if users could get a list of the bindings implemented in the version that they are using.

I understand that core itself may have no need for this feature, but this info is very useful for specific tooling (at least it is in my case) and I thought adding it would help others who need it as well.

@bmeck
Copy link
Member

bmeck commented Jul 15, 2015

I know binding('natives') is used to fast path and prevent overriding natives, with the new loaded bindings wouldn't this make sense to do the same check but on bindings?

@thlorenz
Copy link
Contributor Author

If adding this to process.binding is the problem would it be preferable to expose this information in another way?
Don't want to pollute process though, so not sure where else it could go.

@Qard
Copy link
Member

Qard commented Jul 17, 2015

Everything available through process.binding() is intended only for internal use. The API is not guaranteed to be stable.

Userland should not have any reason to be using bindings directly. If they do, they are almost certainly doing the wrong thing and should be making a PR or feature request to expose the thing they need properly.

@thlorenz
Copy link
Contributor Author

I get all the arguments, but if a tool needs that info it'd be very useful to get it this way instead of hard coding it somewhere or traversing the src folder and guessing which of these are bindings.
There is actually a module out there (can't find it currently) that's just JSON files containing a list of core modules and bindings.
Basically I want to avoid having to resort to those methods via a simple change here.

However if no one else has that need I'm ok with closing this PR and get that info the hacky way :(

@vkurchatkin
Copy link
Contributor

My primary concern is that adding something to bindings like this makes it look like this is actually supposed to be used. Making this public is also not an option, because having a public list of internal stuff doesn't make much sense

@Qard
Copy link
Member

Qard commented Jul 21, 2015

But why would a tool need that? I'm not seeing the use case. Do you have something in particular in mind?

(Not trying to discount the value, just saying that I'm not seeing what exactly the value is.)

@bmeck
Copy link
Member

bmeck commented Jul 21, 2015

I for one like the ability of having check if something exists if I use process.binding (lets face it people do use it / it does not have an underscore). I am sure some APM out there is doing things with process bindings, but lack real world examples.

@jasnell
Copy link
Member

jasnell commented Nov 16, 2015

@thlorenz ... is this still something you'd like to pursue?

@jasnell jasnell added the stalled Issues and PRs that are stalled. label Nov 16, 2015
@jasnell
Copy link
Member

jasnell commented Mar 22, 2016

Closing due to lack of activity or response.

@jasnell jasnell closed this Mar 22, 2016
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++. stalled Issues and PRs that are stalled.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants