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

Integrating timeouts #3

Merged
merged 4 commits into from
Apr 1, 2022
Merged

Integrating timeouts #3

merged 4 commits into from
Apr 1, 2022

Conversation

CMCDragonkai
Copy link
Member

@CMCDragonkai CMCDragonkai commented Mar 31, 2022

Description

This brings in the timeout parameter to locking. This enables us to wait for a certain amount of time before giving up, and in such a case, can help resolve deadlocks after a random jitter.

Issues Fixed

Tasks

  • 1. Integrated into Lock
  • 2. Integrated into RWLockWriter
  • 3. Integrated into RWLockReader

Final checklist

  • Domain specific tests
  • Full tests
  • Updated inline-comment documentation
  • Lint fixed
  • Squash and rebased
  • Sanity check the final build

@CMCDragonkai
Copy link
Member Author

There's a problem here with the RWLockReader.

There's a timing issue with the isLocked(). It ultimately check if the semaphore value is <= 0. Which it is when I check it after having 2 timed out generator locks. But if I wait 0 seconds, it will turn out to be false.

This doesn't appear to affect the RWLockWriter for some reason.

But I can see that it should really be unlocked at that point, but it isn't. So it's a timing issue.

We could do something to "delay" the release somewhat, or delay at the end of the releasing function after calling release() but this does seem like an upstream bug.

@CMCDragonkai
Copy link
Member Author

Seems like it should be possible to reproduce with just any lock, and then 2 timed out attempts to get the lock, and checking isLocked. Going to try this tomorrow and report upstream if so.

@CMCDragonkai
Copy link
Member Author

The lack of timeout support in waitForUnlock required us to create our own subtraction of time for different code sections. DirtyHairy/async-mutex#53

@CMCDragonkai
Copy link
Member Author

I've reproduced the timing bug and posted upstream: DirtyHairy/async-mutex#54

A quickfix for now is to do await sleep(0); at the end of my ResourceRelease, this would ensure that any operations will run afterwards.

@CMCDragonkai
Copy link
Member Author

After reviewing https://javascript.info/event-loop, I think we should use queueMicrotask instead. This should mean that we end up processing any microtasks PLUS any process.nextTick tasks. Some prototyping shows that this works.

A new utility function can be created for this called yieldMicro. Awaiting this ensures all next tick tasks and microtasks are completed before the next line.

// do stuff...
await yieldMicro();

Compared to await sleep(0); it doesn't wait for the entire queue of macro tasks to be completed.

@CMCDragonkai
Copy link
Member Author

Note that without cancellable promises, some of the timeout usages does leave dangling noop promises around. This is not going to be solved until ES7 or JS comes up with a proper way to cancel promises. I did find some interesting packages for this though: MatrixAI/Polykey#297 (comment)

It shouldn't be a huge issue. The noop promises will just be GCed.

@CMCDragonkai
Copy link
Member Author

An unexpected discovery. By using yieldMicro, we're able to fix the order of operations. Now read1 is always the first lock acquired, because read2 and subsequently will yield until all microtasks are done which includes the first reader's promise because promises are microtasks.

@CMCDragonkai
Copy link
Member Author

With the PromiseCancellable, noop promises can be properly cleared.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

Integrate timeouts into lock acqusition
1 participant