vm: adds runInModuleContext() #4955

eljefedelrodeodeljefe · 2016-01-29T18:05:13Z

With regards to nodejs/node-v0.x-archive#9211, this SO question and in order to prevent a signature like the below, I want to propose vm.runInGlobalContext(). This would make the vm API more clear to users, who expect at least one method vm that behaves exactly like their global context.

const vmResult = vm.runInThisContext(require('module').wrap(`
   const http = require('http');
   http.createServer( (request, response) => {
     response.writeHead(200, {'Content-Type': 'text/plain'});
     response.end('Hello World\\n');
   }).listen(8124);

   console.log('Server running at http://127.0.0.1:8124/');
`))(exports, require, module, __filename, __dirname)

bnoordhuis · 2016-01-29T18:38:46Z

lib/vm.js

+  var script = new Script(require('module').wrap(code), options);
+  return script.runInThisContext(options)(exports, require, module,
+                                                            __filename,
+                                                            __dirname);


That's not 'run in global context', that's 'run in the context of the vm module'.

Wouldn't that be the only possibility to have a similar experience like in global context? What would you suggest?

Maybe runInModuleContext? Although it's not clear what the name "module" refers to.

I tried just not wrapping it, but this but again throw ReferenceError: require is not defined

It's the vm module's module object (as is exports). This function is essentially vm.runInVmContext()...

Renaming would be fine for me. I am just looking for ways to have similar require behaviour...while not having the signature as in the description of course. Anyhow it would just be sugar.(good -> less doc explanation sugar)

Sorry, I meant it's not clear to end users what "module" in the name would refer to. But yea, just passing vm's values through isn't great.

vkurchatkin · 2016-01-29T18:57:14Z

Sorry, but this implementation is incorrect

cjihrig · 2016-01-29T19:02:05Z

You might want to take a look at the real module system (lib/module.js, src/node.js, etc.). Also, the vm functions let you specify a filename, which would probably be the correct value for __filename. Not really sure what you'd want to use for __dirname.

eljefedelrodeodeljefe · 2016-01-29T19:11:41Z

The purpose would be to pass everything from the global scope into the scope of the vm. Implementation-wise I am happy for input. Apart from that, can you advise on any other way to be able to require? I think the above way was derived from a couple of tickets. in the archive repo.

eljefedelrodeodeljefe · 2016-01-29T19:16:51Z

Reference would be this comment

vkurchatkin · 2016-01-29T19:23:45Z

The purpose would be to pass everything from the global scope into the scope of the vm

I don't think it's possible

cjihrig · 2016-01-29T19:23:55Z

None of those variables come from the global scope. Each one has its own unique value in every module. If you want to have access to their values in the current file, you would have to pass them in as arguments.

eljefedelrodeodeljefe · 2016-01-29T19:30:05Z

@vkurchatkin I don't necessarily want global scope, but basically just use require-module-system.

The use case would be to send a fs.readFileSync('someserver.js') string to a remote client and run the server in the same process and same thread as the client is running in. It should then be able to require http and spawn a server. Calling require though will fail, because the context has no global variable require, without passing it to it. I thought .runInThisContext() would already be doing this, but the open issues would show, that there is confusion.

vkurchatkin · 2016-01-29T19:45:16Z

I don't necessarily want global scope, but basically just use require-module-system.

it would be confusing. require needs proper file path to work

cjihrig · 2016-01-29T21:25:42Z

@eljefedelrodeodeljefe I put together vm.runInModuleContext(), in cjihrig@befd2c2. I doubt there will be much interest to have it in core (I'll PR it if there is), but it would do what you're looking for. Unfortunately, it relies on 'internal/module', so it's not very convenient for userland either (although it could be adapted).

eljefedelrodeodeljefe · 2016-01-30T00:21:49Z

Okay. Edited this especially the name ~~(edit: not updated in docs)~~, since I have a feeling that I didn't make the purpose of it clear enough. I think what you @cjihrig did is exactly what I was writing, but not what I meant. Also I agree that this then shouldn't be in core. However, I still believe that having a bad API like below, over the latter implementation, should be worth the change (really badly). Speaking out of practice, I really can't teach devs require('module')-wrap-iife-bang, just to let them write js in a vm like they write outside of it, namely requiring native modules.

  const vmResult = vm.runInThisContext(require('module').wrap(`
  const http = require('http');
  http.createServer( (request, response) => {
    response.writeHead(200, {'Content-Type': 'text/plain'});
    response.end('Hello World\\n');
  }).listen(8124);

  console.log('Server running at http://127.0.0.1:8124/');

  `))(exports, require, module)

const vm = require('vm')

const vmResult = vm.runInModuleContext(`
  const http = require('http');
  http.createServer( (request, response) => {
    response.writeHead(200, {'Content-Type': 'text/plain'});
    response.end('Hello World\\n');
  }).listen(8124);

  console.log('Server running at http://127.0.0.1:8124/');
`)

bnoordhuis · 2016-01-30T17:43:04Z

@eljefedelrodeodeljefe When exactly would one use that? The module system used to have an undocumented switch to load each module in a separate vm context but, besides being badly broken for most of its existence, I don't think there really ever was a good use case for. I've never seen one, at least.

(In case you're wondering why the switch existed in the first place: it was added very early on in node's life, for no real reason other than 'because we can'.)

eljefedelrodeodeljefe · 2016-01-30T17:50:59Z

@bnoordhuis I am hacking on a cluster implementation that would support child_processes, detached local process and remote clients. All of them should be able to register at the master node and get instructions which code to run as string via dgram. The current cluster implementation would only support the child process. The idea was to use vm in order not to use the filesystem and not to spawn just a another process, since I can use the worker process and thread right away.

eljefedelrodeodeljefe · 2016-01-30T17:56:54Z

So basically doing this meta programming would also be possible with the solution I am proposing right in vm now just doing userland. It's extremely bulky though. I reckoned that this might be really useful for a lot of use cases and people. This might also render the vm module API more friendly to users and hence propagate the use of it.

vkurchatkin · 2016-01-30T18:10:08Z

This implementation is not really useful. Why would someone want vm's exports or module objects? Why would some want require that is only capable of loading built in modules? You can just use eval if that's what you need

eljefedelrodeodeljefe · 2016-01-30T18:27:47Z

Well eval() use seems to be an anti-pattern to me in general in js and it's behaviour will likely directly depend on the v8 implementation (#2245). Also It allows no sandboxing and options whatsoever. If I could to this without passing anything I'd be happy. It looks like a hack and probably it is also one, but at least it works and would make meta programming js less bulky. I was really (negatively) surprised about vm not being able to require things, to be honest, since it's a very easy step. Those kind of things make up for a lot of tickets and SO Q&As. So I'd advocate: Even if there is pain in it, we should have it for dev-education purposes.

vkurchatkin · 2016-01-30T18:42:14Z

Also It allows no sandboxing and options whatsoever. If I could to this without passing anything I'd be happy

sandboxing means to give explicit access to objects. That's exactly what you are trying to avoid.

Even if there is pain in it, we should have it for dev-education purposes.

Have what? Current implementation will only cause more confusion

eljefedelrodeodeljefe · 2016-01-30T18:47:44Z

Sorry, but I find telling people to do this (below) or eval() really really bad just in order to run some node code:

const vmResult = vm.runInThisContext(require('module').wrap(`
   // code 
`))(exports, require, module)

And still I don't see much pointing against the implementation.

Re: Sandboxing: I am not avoiding it. I just (and this my sole purpose) want to require modules like http. Wherever it might be confusing it might also jsut be the name runInModuleContext which is evenly bad to all the rest. The way vm's API and documentation is currently, I'd rather regard this as a internal module.

vkurchatkin · 2016-01-30T18:53:28Z

Your implementation is not equivalent to this code. It doesn't give access to current modules exports, require and module. It doesn't give access to local variables ('eval' does). It has nothing to do with sandboxing. If you permit require it basically means you permit everything.

eljefedelrodeodeljefe · 2016-01-30T18:59:33Z

Yes and that's fine for me. I don't want to share anything, but run a server. Wherever the exports, modules and requires are coming from, I don't mind, since I just want to use them, all security over board. Security in my use case wouldn't be necessary, since trusting code wouldn't be of concern here.

I also have no intimate knowledge of those modules, but this doesn't seem necessary in this context to me. If there is a non-eval way for running node.js code, please feel free to tell me.

vkurchatkin · 2016-01-30T19:05:51Z

Yes and that's fine for me

That's not a very good reason to include this in core.

Security in my use case wouldn't be necessary, since trusting code wouldn't be of concern here.

So, eval is fine then

eljefedelrodeodeljefe · 2016-01-30T19:16:00Z

Updated the branch. Apparently it's not necessary to pass the module object.

No, because eval() being generally an js anti-pattern. And for other reasons I was already describing above -.-

Again. Having an API with function signatures like above and on open tickets in the archive repo, is worse then adding a couple of symbols to core, imo.

If there is no consensus, I'll be fine. But this leaves me a little bit in disbelieve, tbh.

vkurchatkin · 2016-01-30T19:40:05Z

Ok, I'm done arguing. Here are the problems with your code:

Doesn't work:

vm.runInModuleContext(`
   var express = require('express');
 `)

Doesn't work:

var a = 1;
vm.runInModuleContext(`
   console.log(a);
 `)

Doesn't work:

vm.runInModuleContext(`
   exports.foo = 'bar';
 `)

Works, but it shouldn't

vm.runInModuleContext(`
   require('internal/module');
 `)

eljefedelrodeodeljefe · 2016-01-30T19:58:26Z

See, that is more productive. While case 2 and 3 are out my scope, 4 should not be possible and for, this should be made equivalent:

(while the first works the second does not yet, but I put some work into it)

vm.runInThisContext(require('module').wrap(`
   var express = require('express');
   var app = express();
   app.get('/', function(req, res){
      res.send('hello world');
    });

    app.listen(3000);
 `))(exports, require)


 vm.runInModuleContext(`
    var express = require('express');
    var app = express();
    app.get('/', function(req, res){
       res.send('hello world 2');
     });

     app.listen(3001);
  `)

eljefedelrodeodeljefe · 2016-01-30T21:22:28Z

@vkurchatkin worked on this. I think the update factors it all in. However one would need to pass it ass options, which might also be a good idea in the end.

vm.runInThisContext(`
  console.log(this);
 `)

let options = {
  passThrough: [ exports, require ]
}

var a = 1;
vm.runInModuleContext(`
  var express = require('express');
  var app = express();
  app.get('/', function(req, res){
    res.send('hello world 2');
  });
  app.listen(3001);

  exports.foo = 'bar'; // -> works
  // require('internal/module'); -> does not work, as expected
  // console.log(a);  -> does not work, but is okay
  console.log(this); // should be same as in runInThisContext()
`, options)

bnoordhuis · 2016-01-30T23:23:24Z

To be honest, I don't really see the value of this new method if you end up calling vm.runInThisContext() in the end anyway, it's basically non-bareword eval().

You mention you want to use it in a kind of in-memory cluster but I have trouble seeing what it would buy you.

eljefedelrodeodeljefe · 2016-01-30T23:33:39Z

Hm. Okay, so be it. No, the point would be to run it in another machine. However this was just the opportunity for me to find the vm API really bulky. The proposed change would not be a technical addition, but API enhancement. Yes, the effect is basically eval(), but without all the features vm has and w/o telling js developers they should use something they shouldn't.

This discussion was an attrition, shouldn't have started it. I'll just use the ugly code above.

The intention behind is to present the user a way to execute code in a vm context. The current API doesn't allow this out-of-the-box, since it is neither passing a require function nor creating context with one. The missing docs for this behaviour have produced a number of Q&A items and have also been discussed in the node-archive repo. In both cases there was no real canonical answer. Refs: nodejs/node-v0.x-archive#9211, #4955 PR-URL: #5323 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Stephen Belanger <[email protected]> Reviewed-By: Myles Borins <[email protected]> Reviewed-By: Sakthipriyan Vairamani <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]>

The intention behind is to present the user a way to execute code in a vm context. The current API doesn't allow this out-of-the-box, since it is neither passing a require function nor creating context with one. The missing docs for this behaviour have produced a number of Q&A items and have also been discussed in the node-archive repo. In both cases there was no real canonical answer. Refs: nodejs/node-v0.x-archive#9211, nodejs#4955 PR-URL: nodejs#5323 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Stephen Belanger <[email protected]> Reviewed-By: Myles Borins <[email protected]> Reviewed-By: Sakthipriyan Vairamani <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]>

The intention behind is to present the user a way to execute code in a vm context. The current API doesn't allow this out-of-the-box, since it is neither passing a require function nor creating context with one. The missing docs for this behaviour have produced a number of Q&A items and have also been discussed in the node-archive repo. In both cases there was no real canonical answer. Refs: nodejs/node-v0.x-archive#9211, #4955 PR-URL: #5323 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Stephen Belanger <[email protected]> Reviewed-By: Myles Borins <[email protected]> Reviewed-By: Sakthipriyan Vairamani <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]>

mscdex added the vm Issues and PRs related to the vm subsystem. label Jan 29, 2016

bnoordhuis reviewed Jan 29, 2016
View reviewed changes

eljefedelrodeodeljefe changed the title ~~vm: adds runInGlobalContext()~~ vm: adds runInModuleContext() Jan 30, 2016

vm: adds runInModuleContext()

9b5bfc8

eljefedelrodeodeljefe closed this Jan 30, 2016

eljefedelrodeodeljefe mentioned this pull request Feb 19, 2016

doc: vm, run http server in vm by requiring #5323

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

vm: adds runInModuleContext() #4955

vm: adds runInModuleContext() #4955

eljefedelrodeodeljefe commented Jan 29, 2016

bnoordhuis Jan 29, 2016

eljefedelrodeodeljefe Jan 29, 2016

cjihrig Jan 29, 2016

eljefedelrodeodeljefe Jan 29, 2016

bnoordhuis Jan 29, 2016

eljefedelrodeodeljefe Jan 29, 2016

cjihrig Jan 29, 2016

vkurchatkin commented Jan 29, 2016

cjihrig commented Jan 29, 2016

eljefedelrodeodeljefe commented Jan 29, 2016

eljefedelrodeodeljefe commented Jan 29, 2016

vkurchatkin commented Jan 29, 2016

cjihrig commented Jan 29, 2016

eljefedelrodeodeljefe commented Jan 29, 2016

vkurchatkin commented Jan 29, 2016

cjihrig commented Jan 29, 2016

eljefedelrodeodeljefe commented Jan 30, 2016

bnoordhuis commented Jan 30, 2016

eljefedelrodeodeljefe commented Jan 30, 2016

eljefedelrodeodeljefe commented Jan 30, 2016

vkurchatkin commented Jan 30, 2016

eljefedelrodeodeljefe commented Jan 30, 2016

vkurchatkin commented Jan 30, 2016

eljefedelrodeodeljefe commented Jan 30, 2016

vkurchatkin commented Jan 30, 2016

eljefedelrodeodeljefe commented Jan 30, 2016

vkurchatkin commented Jan 30, 2016

eljefedelrodeodeljefe commented Jan 30, 2016

vkurchatkin commented Jan 30, 2016

eljefedelrodeodeljefe commented Jan 30, 2016

eljefedelrodeodeljefe commented Jan 30, 2016

bnoordhuis commented Jan 30, 2016

eljefedelrodeodeljefe commented Jan 30, 2016

vm: adds runInModuleContext() #4955

vm: adds runInModuleContext() #4955

Conversation

eljefedelrodeodeljefe commented Jan 29, 2016

bnoordhuis Jan 29, 2016

Choose a reason for hiding this comment

eljefedelrodeodeljefe Jan 29, 2016

Choose a reason for hiding this comment

cjihrig Jan 29, 2016

Choose a reason for hiding this comment

eljefedelrodeodeljefe Jan 29, 2016

Choose a reason for hiding this comment

bnoordhuis Jan 29, 2016

Choose a reason for hiding this comment

eljefedelrodeodeljefe Jan 29, 2016

Choose a reason for hiding this comment

cjihrig Jan 29, 2016

Choose a reason for hiding this comment

vkurchatkin commented Jan 29, 2016

cjihrig commented Jan 29, 2016

eljefedelrodeodeljefe commented Jan 29, 2016

eljefedelrodeodeljefe commented Jan 29, 2016

vkurchatkin commented Jan 29, 2016

cjihrig commented Jan 29, 2016

eljefedelrodeodeljefe commented Jan 29, 2016

vkurchatkin commented Jan 29, 2016

cjihrig commented Jan 29, 2016

eljefedelrodeodeljefe commented Jan 30, 2016

bnoordhuis commented Jan 30, 2016

eljefedelrodeodeljefe commented Jan 30, 2016

eljefedelrodeodeljefe commented Jan 30, 2016

vkurchatkin commented Jan 30, 2016

eljefedelrodeodeljefe commented Jan 30, 2016

vkurchatkin commented Jan 30, 2016

eljefedelrodeodeljefe commented Jan 30, 2016

vkurchatkin commented Jan 30, 2016

eljefedelrodeodeljefe commented Jan 30, 2016

vkurchatkin commented Jan 30, 2016

eljefedelrodeodeljefe commented Jan 30, 2016

vkurchatkin commented Jan 30, 2016

eljefedelrodeodeljefe commented Jan 30, 2016

eljefedelrodeodeljefe commented Jan 30, 2016

bnoordhuis commented Jan 30, 2016

eljefedelrodeodeljefe commented Jan 30, 2016