stronger checking for functions in client.js

[arcana261]

checking for functions simply by instanceof would render library usesless in vm or REPL contexts. because if client is created in another V8 context, typeof would still return "function" but instanceof Function would fail and return false for functions and arrow functions. thus it would be impossible to create client before starting a REPL context.

Mainly I ran into problem using grpcc with following error:

Error: Argument mismatch in makeClientStreamRequest
    at ServiceClient.Client.makeClientStreamRequest (/usr/local/lib/node_modules/grpcc/node_modules/grpc/src/client.js:640:11)
    at apply (/usr/local/lib/node_modules/grpcc/node_modules/lodash/lodash.js:470:17)
    at ServiceClient.wrapper [as add] (/usr/local/lib/node_modules/grpcc/node_modules/lodash/lodash.js:5329:16)

After inspecting the problem and reading node docs about V8 contexts and so forth I concluded that "instanceof" is not evaluated properly because when a callback is created in REPL context, it is treated as when functions are created in another window and instanceof would fail.

[thelinuxfoundation]

Thank you for your pull request. Before we can look at your contribution, we need to ensure all contributors are covered by a Contributor License Agreement.

After the following items are addressed, please respond with a new comment here, and the automated system will re-verify.

• User @arcana261 isn't covered by a CLA. They will need to complete the form at https://identity.linuxfoundation.org/projects/cncf

Regards,
CLA GitHub bot

[arcana261]

I've signed CLA

[murgatroid99]

Can you share reproduction steps for this problem?

[murgatroid99]

Also, since we are already depending on lodash anyway, I would prefer to just use _.isFunction, rather than take a new dependency.

[arcana261]

Dear good sir.. The following example fails with error message Argument Mismatch

const grpc = require('grpc');
const f = '../proto/proto/math.proto';
const vm = require('vm');

parsed = grpc.load(f);

const service = new parsed.math.MathService('localhost:8000',  grpc.credentials.createInsecure());
const sandbox = { service: service };
vm.createContext(sandbox);

script = `
ch = service.add((err, data) => console.log(err, data));
ch.write(1);
ch.write(5);
ch.end();
`;

vm.runInContext(script, sandbox);

[arcana261]

A more general example of V8 contexts and behavior of instanceof Function would be:

const vm = require('vm');

const sandbox = {
  isFunction: f => f instanceof Function,
  result: null
};
vm.createContext(sandbox);

// prints:
// outof sandbox: true
console.log('outof sandbox:', sandbox.isFunction(() => console.log()));

script = `
result = isFunction(() => console.log());
`;
vm.runInContext(script, sandbox);

// prints:
// inside sandbox: false
console.log('inside sandbox:', sandbox.result);

[arcana261]

I apologize for not noticing that.. I've replaced usage of isFunction with _.isFunction

[murgatroid99]

Thank you for your contribution. This change will definitely go into the 1.11.x release, and we may decide to backport it to 1.10.x.