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

Make sandbox concurrency safe #2575

Closed
wants to merge 2 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 6 additions & 4 deletions lib/sinon/proxy-invoke.js
Original file line number Diff line number Diff line change
Expand Up @@ -2,18 +2,20 @@

const arrayProto = require("@sinonjs/commons").prototypes.array;
const proxyCallUtil = require("./proxy-call-util");
const globalContext = require("./util/core/global-context");

const push = arrayProto.push;
const forEach = arrayProto.forEach;
const concat = arrayProto.concat;
const ErrorConstructor = Error.prototype.constructor;
const bind = Function.prototype.bind;

let callId = 0;

module.exports = function invoke(func, thisValue, args) {
module.exports = function invoke(func, thisValue, args, ctx = globalContext) {
const matchings = this.matchingFakes(args);
const currentCallId = callId++;
if (ctx.callId == null) {

Check failure on line 15 in lib/sinon/proxy-invoke.js

View workflow job for this annotation

GitHub Actions / lint

Use '===' to compare with null
ctx.callId = 0;

Check warning on line 16 in lib/sinon/proxy-invoke.js

View check run for this annotation

Codecov / codecov/patch

lib/sinon/proxy-invoke.js#L16

Added line #L16 was not covered by tests
}
Comment on lines +15 to +17
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When does this ever happen? I cannot see any tests exercising this.

I see the fallback global module, where it is already initialised, and I see the sandbox, which is also initialised.

const currentCallId = ctx.callId++;
let exception, returnValue;

proxyCallUtil.incrementCallCount(this);
Expand Down
35 changes: 18 additions & 17 deletions lib/sinon/proxy.js
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@

const arrayProto = require("@sinonjs/commons").prototypes.array;
const extend = require("./util/core/extend");
const globalContext = require("./util/core/global-context");
const functionToString = require("./util/core/function-to-string");
const proxyCall = require("./proxy-call");
const proxyCallUtil = require("./proxy-call-util");
Expand Down Expand Up @@ -240,8 +241,8 @@
delegateToCalls(proxyApi, "calledWithNew", true);
delegateToCalls(proxyApi, "alwaysCalledWithNew", false, "calledWithNew");

function createProxy(func, originalFunc) {
const proxy = wrapFunction(func, originalFunc);
function createProxy(func, originalFunc, ctx = globalContext) {
const proxy = wrapFunction(func, originalFunc, ctx);

// Inherit function properties:
extend(proxy, func);
Expand All @@ -253,7 +254,7 @@
return proxy;
}

function wrapFunction(func, originalFunc) {
function wrapFunction(func, originalFunc, ctx = globalContext) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why are all of these context objects optional, with a default global? Even the default Sinon instance is a sandbox in itself.

const arity = originalFunc.length;
let p;
// Do not change this to use an eval. Projects that depend on sinon block the use of eval.
Expand All @@ -262,72 +263,72 @@
/*eslint-disable no-unused-vars, max-len*/
case 0:
p = function proxy() {
return p.invoke(func, this, slice(arguments));
return p.invoke(func, this, slice(arguments), ctx);
};
break;
case 1:
p = function proxy(a) {
return p.invoke(func, this, slice(arguments));
return p.invoke(func, this, slice(arguments), ctx);
};
break;
case 2:
p = function proxy(a, b) {
return p.invoke(func, this, slice(arguments));
return p.invoke(func, this, slice(arguments), ctx);
};
break;
case 3:
p = function proxy(a, b, c) {
return p.invoke(func, this, slice(arguments));
return p.invoke(func, this, slice(arguments), ctx);

Check warning on line 281 in lib/sinon/proxy.js

View check run for this annotation

Codecov / codecov/patch

lib/sinon/proxy.js#L281

Added line #L281 was not covered by tests
};
break;
case 4:
p = function proxy(a, b, c, d) {
return p.invoke(func, this, slice(arguments));
return p.invoke(func, this, slice(arguments), ctx);
};
break;
case 5:
p = function proxy(a, b, c, d, e) {
return p.invoke(func, this, slice(arguments));
return p.invoke(func, this, slice(arguments), ctx);

Check warning on line 291 in lib/sinon/proxy.js

View check run for this annotation

Codecov / codecov/patch

lib/sinon/proxy.js#L291

Added line #L291 was not covered by tests
};
break;
case 6:
p = function proxy(a, b, c, d, e, f) {
return p.invoke(func, this, slice(arguments));
return p.invoke(func, this, slice(arguments), ctx);

Check warning on line 296 in lib/sinon/proxy.js

View check run for this annotation

Codecov / codecov/patch

lib/sinon/proxy.js#L296

Added line #L296 was not covered by tests
};
break;
case 7:
p = function proxy(a, b, c, d, e, f, g) {
return p.invoke(func, this, slice(arguments));
return p.invoke(func, this, slice(arguments), ctx);

Check warning on line 301 in lib/sinon/proxy.js

View check run for this annotation

Codecov / codecov/patch

lib/sinon/proxy.js#L301

Added line #L301 was not covered by tests
};
break;
case 8:
p = function proxy(a, b, c, d, e, f, g, h) {
return p.invoke(func, this, slice(arguments));
return p.invoke(func, this, slice(arguments), ctx);

Check warning on line 306 in lib/sinon/proxy.js

View check run for this annotation

Codecov / codecov/patch

lib/sinon/proxy.js#L306

Added line #L306 was not covered by tests
};
break;
case 9:
p = function proxy(a, b, c, d, e, f, g, h, i) {
return p.invoke(func, this, slice(arguments));
return p.invoke(func, this, slice(arguments), ctx);

Check warning on line 311 in lib/sinon/proxy.js

View check run for this annotation

Codecov / codecov/patch

lib/sinon/proxy.js#L311

Added line #L311 was not covered by tests
};
break;
case 10:
p = function proxy(a, b, c, d, e, f, g, h, i, j) {
return p.invoke(func, this, slice(arguments));
return p.invoke(func, this, slice(arguments), ctx);

Check warning on line 316 in lib/sinon/proxy.js

View check run for this annotation

Codecov / codecov/patch

lib/sinon/proxy.js#L316

Added line #L316 was not covered by tests
};
break;
case 11:
p = function proxy(a, b, c, d, e, f, g, h, i, j, k) {
return p.invoke(func, this, slice(arguments));
return p.invoke(func, this, slice(arguments), ctx);

Check warning on line 321 in lib/sinon/proxy.js

View check run for this annotation

Codecov / codecov/patch

lib/sinon/proxy.js#L321

Added line #L321 was not covered by tests
};
break;
case 12:
p = function proxy(a, b, c, d, e, f, g, h, i, j, k, l) {
return p.invoke(func, this, slice(arguments));
return p.invoke(func, this, slice(arguments), ctx);

Check warning on line 326 in lib/sinon/proxy.js

View check run for this annotation

Codecov / codecov/patch

lib/sinon/proxy.js#L326

Added line #L326 was not covered by tests
};
break;
default:
p = function proxy() {
return p.invoke(func, this, slice(arguments));
return p.invoke(func, this, slice(arguments), ctx);

Check warning on line 331 in lib/sinon/proxy.js

View check run for this annotation

Codecov / codecov/patch

lib/sinon/proxy.js#L331

Added line #L331 was not covered by tests
};
break;
/*eslint-enable*/
Expand Down
10 changes: 8 additions & 2 deletions lib/sinon/sandbox.js
Original file line number Diff line number Diff line change
Expand Up @@ -78,6 +78,7 @@ function checkForValidArguments(descriptor, property, replacement) {
*/
function Sandbox(opts = {}) {
const sandbox = this;
sandbox.callId = 0;
const assertOptions = opts.assertOptions || {};
let fakeRestorers = [];
let promiseLib;
Expand Down Expand Up @@ -445,8 +446,13 @@ function Sandbox(opts = {}) {
return spy;
}

sandbox.spy = function spy() {
const createdSpy = sinonSpy.apply(sinonSpy, arguments);
sandbox.spy = function spy(object, property, types) {
const createdSpy = sinonSpy.apply(sinonSpy, [
object,
property,
types,
sandbox,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you are passing in the sandbox as the context, then what is the need for the global context object?

]);
return commonPostInitSetup(arguments, createdSpy);
};

Expand Down
19 changes: 12 additions & 7 deletions lib/sinon/spy.js
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ const proxyCallUtil = require("./proxy-call-util");
const walkObject = require("./util/core/walk-object");
const wrapMethod = require("./util/core/wrap-method");
const valueToString = require("@sinonjs/commons").valueToString;
const globalContext = require("./util/core/global-context");

/* cache references to library methods so that they also can be stubbed without problems */
const forEach = arrayProto.forEach;
Expand Down Expand Up @@ -130,7 +131,7 @@ delegateToCalls(
},
);

function createSpy(func) {
function createSpy(func, context = globalContext) {
let name;
let funk = func;

Expand All @@ -142,7 +143,7 @@ function createSpy(func) {
name = functionName(funk);
}

const proxy = createProxy(funk, funk);
const proxy = createProxy(funk, funk, context);

// Inherit spy API:
extend.nonEnum(proxy, spyApi);
Expand All @@ -155,13 +156,13 @@ function createSpy(func) {
return proxy;
}

function spy(object, property, types) {
function spy(object, property, types, context = globalContext) {
if (isEsModule(object)) {
throw new TypeError("ES Modules cannot be spied");
}

if (!property && typeof object === "function") {
return createSpy(object);
return createSpy(object, context);
}

if (!property && typeof object === "object") {
Expand All @@ -171,18 +172,22 @@ function spy(object, property, types) {
if (!object && !property) {
return createSpy(function () {
return;
});
}, context);
}

if (!types) {
return wrapMethod(object, property, createSpy(object[property]));
return wrapMethod(
object,
property,
createSpy(object[property], context),
);
}

const descriptor = {};
const methodDesc = getPropertyDescriptor(object, property);

forEach(types, function (type) {
descriptor[type] = createSpy(methodDesc[type]);
descriptor[type] = createSpy(methodDesc[type], context);
});

return wrapMethod(object, property, descriptor);
Expand Down
5 changes: 5 additions & 0 deletions lib/sinon/util/core/global-context.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
"use strict";

module.exports = {
callId: 0,
};
Comment on lines +1 to +5
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this even needed?

48 changes: 48 additions & 0 deletions test/sandbox-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -2377,4 +2377,52 @@ describe("Sandbox", function () {
sandboxB.restore();
});
});
describe("concurrency safe", function () {
it("spy", async function () {
class F {
constructor() {
this.first = [];
this.second = [];
this.third = [];
}
async run() {
await this.execute("first");
await Promise.resolve();
await this.execute("second");
await Promise.resolve();
await this.execute("third");
}

async execute(s) {
for (const f of this[s]) {
await f();
}
}
Comment on lines +2380 to +2400
Copy link
Contributor

@fatso83 fatso83 Jan 4, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This test was a bit hard to wrap my head around, at least at midnight ... 😄 Could you clarify what you are really doing? I mean, I assume you are somehow testing quazi-concurrent runs, interlacing runs from different sandboxes, but I still find it a bit hard to reason around the order of the calls.

Is there some way you could re-order or refactor this in some way that made the intent as expressed through the code clearer?

To me, it is not at all clear how the sandbox runs are being interlaced and in what order functions are being called. Would it not be much clearer if doing something a la the following pseudo-code?

const sandboxASpy1 = sandboxA.spy();
const sandboxASpy2 = sandboxA.spy();
const sandboxBSpy1 = sandboxB.spy();
const sandboxBSpy2 = sandboxB.spy();

sandboxASpy1();
sandboxBSpy1();
sandboxBSpy2();
sandboxASpy2();

assert(sandboxASpy1.calledImmediatelyBefore(sandboxASpy2));
assert(sandboxBSpy1.calledImmediatelyBefore(sandboxBSpy2));

There is no need for awaits AFAIK. JS is single threaded no matter what, and the asynchronicity just muddles up what is going on IMHO.

}

const fn = async () => {
const sinonSandbox = createSandbox();

const f = new F();

const a = sinonSandbox.spy();
const b = sinonSandbox.spy();
const c = sinonSandbox.spy();

f.first.push(a);
f.second.push(b);
f.third.push(c);

await f.run();

assert(a.calledBefore(b));
assert(b.calledBefore(c));

assert(a.calledImmediatelyBefore(b));
assert(b.calledImmediatelyBefore(c));
};

await Promise.all([fn(), fn(), fn(), fn()]);
});
});
});
Loading