Skip to content

Make sandbox concurrency safe #2575

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.

Sign up for GitHub

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

Closed
wants to merge 2 commits into from
+97 −30
Closed

Make sandbox concurrency safe #2575

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
d2b43cf
use context for callId
remorses Jan 4, 2024
fabf95c
add callId = 0 in sandbox
remorses Jan 4, 2024
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
View file
Open in desktop
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) {
ctx.callId = 0;
}
Comment on lines +15 to +17
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.

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
View file
Open in desktop
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, "alwaysReturned", false, "returned");
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 @@ function createProxy(func, originalFunc) {
return proxy;
}

function wrapFunction(func, originalFunc) {
function wrapFunction(func, originalFunc, ctx = globalContext) {
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.

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 @@ function wrapFunction(func, originalFunc) {
/*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);
};
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);
};
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);
};
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);
};
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);
};
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);
};
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);
};
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);
};
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);
};
break;
default:
p = function proxy() {
return p.invoke(func, this, slice(arguments));
return p.invoke(func, this, slice(arguments), ctx);
};
break;
/*eslint-enable*/
Expand Down
10 changes: 8 additions & 2 deletions lib/sinon/sandbox.js
View file
Open in desktop
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

@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.

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
View file
Open in desktop
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
View file
Open in desktop
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

@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.

Is this even needed?

48 changes: 48 additions & 0 deletions test/sandbox-test.js
View file
Open in desktop
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 •
edited
Loading

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()]);
});
});
});