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.

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

Jump to bottom

Locking timeout #24

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open

Locking timeout #24

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
b46ab15
feat: locking timeout
anton-iskryzhytskyi Mar 15, 2020
07c7387
fix: remove timeout from Lock and add it to Lock.queue element
anton-iskryzhytskyi Mar 16, 2020
f9099d4
refactor: add the clear timeout logic
anton-iskryzhytskyi Mar 16, 2020
364e6b7
refactor: move errors to separate file
anton-iskryzhytskyi Mar 16, 2020
ed212e4
feat: clean timeout timer on abort, throw error on timeout
anton-iskryzhytskyi Mar 16, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
1 change: 1 addition & 0 deletions test.js
View file
Open in desktop
Expand Up @@ -8,6 +8,7 @@ const tests = [
'deadlock',
'recursive-deadlock',
'thread-main',
'lock-time',
];

(async () => {
Expand Down
18 changes: 18 additions & 0 deletions test/lock-time.js
View file
Open in desktop
@@ -0,0 +1,18 @@
'use strict';

const assert = require('assert').strict;
const { locks } = require('..');
const { sleep } = require('./test-utils');
anton-iskryzhytskyi marked this conversation as resolved.
Show resolved Hide resolved

const TIME_TO_PROCESS = 200;
const TIME_TO_LOCK = 100;

module.exports = async () => {
const startTs = Date.now();

await locks.request('LockTime', { timeout: TIME_TO_LOCK }, async () => {
await sleep(TIME_TO_PROCESS);
});

assert.strictEqual(Date.now() - startTs < TIME_TO_PROCESS, true);
};
8 changes: 1 addition & 7 deletions test/steps.js
View file
Open in desktop
Expand Up @@ -2,13 +2,7 @@

const assert = require('assert').strict;
const { locks } = require('..');

const sleep = msec =>
new Promise(resolve => {
setTimeout(() => {
resolve();
}, msec);
});
const { sleep } = require('./test-utils');
anton-iskryzhytskyi marked this conversation as resolved.
Show resolved Hide resolved

let counter = 0;

Expand Down
10 changes: 10 additions & 0 deletions test/test-utils.js
View file
Open in desktop
@@ -0,0 +1,10 @@
'use strict';

const sleep = msec =>
new Promise(resolve => {
setTimeout(resolve, msec);
});

module.exports = {
sleep,
};
6 changes: 1 addition & 5 deletions test/thread-worker.js
View file
Open in desktop
@@ -1,14 +1,10 @@
'use strict';

const threads = require('worker_threads');
const { sleep } = require('./test-utils');
anton-iskryzhytskyi marked this conversation as resolved.
Show resolved Hide resolved

const { locks } = require('..');

const sleep = msec =>
new Promise(resolve => {
setTimeout(resolve, msec);
});

(async () => {
if (threads.threadId === 2) {
await sleep(10);
Expand Down
20 changes: 13 additions & 7 deletions web-locks.js
View file
Open in desktop
Expand Up @@ -22,9 +22,9 @@ class Lock {
if (!buffer) Atomics.store(this.flag, 0, UNLOCKED);
}

enter(handler) {
enter(handler, timeout) {
Copy link
Member

Choose a reason for hiding this comment

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

That it much better, but we need to compare passing timeout and timer instance.

Copy link
Author

Choose a reason for hiding this comment

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

Maybe we need to use clearTimeout on abort?

Hmm, maybe it would be better to throw an error on timeout, similar to abort behavior?
Also, move all resolve / reject invocations to one place.

return new Promise(resolve => {
this.queue.push({ handler, resolve });
this.queue.push({ handler, resolve, timeout });
this.trying = true;
setTimeout(() => {
this.tryEnter();
Expand All @@ -38,11 +38,17 @@ class Lock {
if (prev === LOCKED) return;
this.owner = true;
this.trying = false;
const { handler, resolve } = this.queue.shift();
handler(this).finally(() => {
const { handler, resolve, timeout } = this.queue.shift();

const endWork = () => {
anton-iskryzhytskyi marked this conversation as resolved.
Show resolved Hide resolved
this.leave();
resolve();
});
};

if (timeout) {
setTimeout(endWork, timeout);
}
handler(this).finally(endWork);
anton-iskryzhytskyi marked this conversation as resolved.
Show resolved Hide resolved
}

leave() {
Expand Down Expand Up @@ -89,7 +95,7 @@ class LockManager {
handler = options;
options = {};
}
const { mode = 'exclusive', signal = null } = options;
const { mode = 'exclusive', signal = null, timeout } = options;

let lock = this.collection.get(name);
if (!lock) {
Expand All @@ -100,7 +106,7 @@ class LockManager {
locks.send(message);
}

const finished = lock.enter(handler);
const finished = lock.enter(handler, timeout);
let aborted = null;
if (signal) {
aborted = new Promise((resolve, reject) => {
Expand Down