t-moe / FluentFlow

A tool to filter json objects by describing their (timely) behaviour
GNU General Public License v3.0
4 stars 1 forks source link

Deasync does not work in NodeVm #7

Closed t-moe closed 7 years ago

t-moe commented 8 years ago

Running npm test on 4183790 gives me:

VMError: Module 'C:\Users\Timo\FluentFlow\node_modules\deasync\build\deasync.node' not found
    at _require (sandbox.js:138:17)
    at bindings (C:\Users\Timo\FluentFlow\node_modules\deasync\node_modules\bindings\bindings.js:76:44)
    at C:\Users\Timo\FluentFlow\node_modules\deasync\index.js:23:31
    at _require (sandbox.js:189:9)
    at C:\Users\Timo\FluentFlow\core\matcher.js:5:15
    at _require (sandbox.js:189:9)
    at C:\Users\Timo\FluentFlow\core\fluent.js:5:15
    at _require (sandbox.js:189:9)
    at C:\Users\Timo\FluentFlow\core\matchboxenv.js:1:92
    at _require (sandbox.js:189:9)

This error occours on linux and on windows.

The probem seems to be that in deasync/index.js:20 the require fails (when executed in NodeVM, otherwise it's fine). Here's an internal error message (similar to the one on linux):

Failed to load 'C:\Users\Timo\FluentFlow\node_modules\deasync\bin\win32-x64-node-4\deasync.node': [Module did not self-register.]

Not sure what this [Module did not self-register.] means.

t-moe commented 8 years ago

Hmm, cannot reproduce this outside of fluentflow.

One could think that the following snippet should fail, but it doesn't:

    const NodeVM = require('vm2').NodeVM;
    const vm = new NodeVM({
        require: true,
        requireExternal: true,
        requireNative: ['fs','path']
    });

    vm.run("module.exports = require('deasync')(require('fs').readFile)", __filename);

How is this different from what we do in FluentFlow with matchbox/matchboxenv?

t-moe commented 8 years ago

@Enteee Could you please take a look at this? thx FYI: Works fine when using no-vm.

Enteee commented 8 years ago

This fails because bindings checks error codes based on strings and vm2 has it's own exceptions which do not match the real ones

more detail:

break in node_modules/bindings/bindings.js:78
 76     tries.push(n)
 77     try {
>78       b = opts.path ? require.resolve(n) : require(n)
 79       if (!opts.path) {
 80         b.path = n
debug> repl
Press Ctrl + C to leave debug repl
> opts.path
> n
'/home/ente/workspace/fluentflow/node_modules/deasync/build/deasync.node'
break in node_modules/bindings/bindings.js:84
 82       return b
 83     } catch (e) {
>84       if (!/not find/i.test(e.message)) {
 85         throw e
 86       }
debug> repl
Press Ctrl + C to leave debug repl
> e
{ name: 'VMError',
  message: 'Module \'/home/ente/workspace/fluentflow/node_modules/deasync/build/deasync.node\'... (length: 90)',
  code: 'ENOTFOUND',
  stack: undefined }

the expected message is probably:

> try { require('thisdoesnotexist') } catch (e) { console.log(e); }
{ [Error: Cannot find module 'thisdoesnotexist'] code: 'MODULE_NOT_FOUND' }

which is bad. As not even the code of the VMError matches the real one.

So what I could do now is:

  1. ask the vm2 guys to use the same codes as the real exceptions
  2. ask the bindings guys to check for the code instead of using regex on the message (which seems to be a bad idea in the first place)

Anyway as this might take a while we might be better of forking the two projects and changing it on our own.

t-moe commented 8 years ago

The point is: line 34 should never be reached. The bindings module should never be used. In normal operation line 31 will succeed. With vm2 line 31 fails, and then it tries line 34 which fails even more.
The bindings module is only used, when using a platform which is not listed here.

t-moe commented 8 years ago

And also: Why does it work if I use the snippet from the 2nd comment from this issue, and it doesn't work if we use our matchbox env? The problem could be on our side....

t-moe commented 8 years ago

finally a minimal, reproducible example which shows the problem outside of fluentflow:

const NodeVM = require('vm2').NodeVM;

const vm = new NodeVM({
    require: true,
    requireExternal: true,
    requireNative: ['fs','path']
});

vm.run("module.exports = require('deasync')(require('fs').readFile)", __filename);

const vm2 = new NodeVM({
    require: true,
    requireExternal: true,
    requireNative: ['fs','path']
});

vm2.run("module.exports = require('deasync')(require('fs').readFile)", __filename);

throws

c:\Users\Timo\FluentFlow\node_modules\deasync\node_modules\bindings\bindings.js:83
        throw e
        ^
VMError: Module 'c:\Users\Timo\FluentFlow\node_modules\deasync\build\deasync.node' not found
    at _require (sandbox.js:138:17)
    at bindings (c:\Users\Timo\FluentFlow\node_modules\deasync\node_modules\bindings\bindings.js:76:44)
    at c:\Users\Timo\FluentFlow\node_modules\deasync\index.js:23:31
    at _require (sandbox.js:189:9)
    at Object.<anonymous> (c:\Users\Timo\FluentFlow\nodeVmBug.js:1:80)
    at NodeVM.run (c:\Users\Timo\FluentFlow\node_modules\vm2\lib\main.js:393:13)
    at Object.<anonymous> (c:\Users\Timo\FluentFlow\nodeVmBug.js:18:5)
    at Module._compile (module.js:409:26)
    at Object.Module._extensions..js (module.js:416:10)
    at Module.load (module.js:343:32)
Enteee commented 8 years ago

why calling vm.run twice?

t-moe commented 8 years ago

otherwise the error doesn't occour :). I suspect that it has something to do with the native module caching. like require.cache.

So yes: we can do exactly one unit test with nodevm, and then everything is fine. we'll never run into a problem :)

Enteee commented 8 years ago

i do think we should move this to the vm2 issues section...

t-moe commented 8 years ago

Opened an issue on vm2: https://github.com/patriksimek/vm2/issues/29

t-moe commented 7 years ago

vm2 fixed the issue in version 3.0