Cross-Platform NamedLocks Reimplementation

[tsavo-at-pieces]

Hey @bsutton - putting this here so that you and I can both start testing this together to make sure everything stays in tact.

You likely know where the breaks would occur better than I but I wanted to share how I initially thought about refactoring dart_project.dart file with the updated NamedLocks guard.

That said - if you're thinking it's really important for end users to still have access to the old NamedLocks class I can re-implement its' internals with the primitives that are found in named_locks pretty easily I believe. The only caveat is that this new NamedLocks system runs on Native Semaphores across Unix and Windows i.e. building on the native_semaphores codebase and doesn't rely on the filesystem i.e. there isn't support for file paths as Named Semaphores are OS wide.

The other thing to note is that right now it supports cross process locking and reentrant behavior within a process. It is able to keep track of the counts reentrantly but it doesn't keep track of reentrant counts across process (yet). If this is needed by DCLI or the community this can be implemented with shared memory or mmap pretty easily.

Let me know thoughts on some of this but most importantly I'm just looking to get your feedback on if it works as expected for the DCLI use case. Perhaps you can pull this branch down and do some testing in your environment and then circle back here.

Let me know thoughts 🙏🤝

Cheers, -Tsavo

[bsutton]

So I would prefer to keep the existing NamedLock api as part of our public api so we don't unnecessarily break peoples code.

If a user wants to use the new guard method then my thinking is that we should direct them to use your package directly - this is also inline with dart recommendations.

[bsutton]

I'm also thinking that we need a new version of NamedLock - NamedLockAsync which allows the callback to be an async function. This is in keeping with the likes of withTempDir/withTempDirAsync.

[bsutton]

runs on Native Semaphores across Unix and Windows i.e. building on the native_semaphores codebase and doesn't rely on the filesystem i.e. there isn't support for file paths as Named Semaphores are OS wide.

The files paths used for the locking are not part of the public api so the fact that you know use a different method is fine.

[bsutton]

The other thing to note is that right now it supports cross process locking and reentrant behavior within a process. It is able to keep track of the counts reentrantly but it doesn't keep track of reentrant counts across process (yet). If this is needed by DCLI or the community this can be implemented with shared memory or mmap pretty easily.

The code should NOT be re-entrant across processes - so the current implementation is fine.

The code should ONLY be re-entrant within an isolate. If two isolates in the same process try to enter a lock, only one is allowed to succeed.

The way that re-entrance work is critical! If I've understood you correctly that multiple isolates in one process can enter the same lock, then we have a problem that will need to be resolved.

[bsutton]

So I'm a little confused - the PR title is linux only but then you state that the underlying implementation works on Windows and Linux?

I assume when you say linux you are including macos as well?

[tsavo-at-pieces]

I'm also thinking that we need a new version of NamedLock - NamedLockAsync which allows the callback to be an async function. This is in keeping with the likes of withTempDir/withTempDirAsync.

Can absolutely do something like this - I actually allow the ExecutableCall callable to be sync or async and async values can be accessed on the completer.future 🙌

Here is the internal code that does this, the completer field is higher up in that class and publicly accessible.

So to access it one could do something like:

    final ExecutionCall<Future<bool>, Exception> _guarded = NamedLock.guard(
      name: name,
      execution: ExecutionCall<Future<bool>, Exception>(
        callable: () async {
          sleep(Duration(milliseconds: Random().nextInt(5000)));
          return Future.sync(true);
        },
      ),
    );

    print(await _guarded.completer.future);

    // or 
    print(await _guarded.returned);

Can absolutely do an NamedLock.guardAsync as well just to be super explicit!

[tsavo-at-pieces]

The other thing to note is that right now it supports cross process locking and reentrant behavior within a process. It is able to keep track of the counts reentrantly but it doesn't keep track of reentrant counts across process (yet). If this is needed by DCLI or the community this can be implemented with shared memory or mmap pretty easily.

The code should NOT be re-entrant across processes - so the current implementation is fine.

The code should ONLY be re-entrant within an isolate. If two isolates in the same process try to enter a lock, only one is allowed to succeed.

The way that re-entrance work is critical! If I've understood you correctly that multiple isolates in one process can enter the same lock, then we have a problem that will need to be resolved.

I believe the re-entrant behavior is as you described above. I have verified that all of the unit tests that you had previously are passing as expected 🤝 I will need to create some unit tests to double check that multiple isolates in one process can not enter the same lock but I am pretty sure this is the case as it stands right now!

Link to passing unit tests comment here

[tsavo-at-pieces]

So I'm a little confused - the PR title is linux only but then you state that the underlying implementation works on Windows and Linux?

I assume when you say linux you are including macos as well?

Yep - this weekend I'll remove that note on unix as Windows is 99% g2g I have one small nit in a unit test that I believe is a flipped boolean somewhere on the underlying named semaphore implementation. That said once that is passing the named locks code will work for windows with no changes needed 🫡

[bsutton]

I'm at the point that if you can get you code merged that we do a 4.0 release.

They are still a few dangling issue but I think we have the core paths that most people use working.

As such I'm inclined to not let perfect get in the way of good enough.

Thoughts?

On Sun, 12 May 2024, 8:18 am Tsavo Knott, @.***> wrote:

@.**** commented on this pull request.

In dcli/lib/src/script/dart_project.dart https://github.com/onepub-dev/dcli/pull/246#discussion_r1597515015:

@@ -217,11 +210,11 @@ class DartProject { return null; }

• NamedLock? __lock;
• static const _lockName = 'script.lock';
• // NamedLock? __lock;
• static const _lockName = 'dcli.script.lock.a1';

yes sir! I took the a1 off though as that was just a quick name change to run it on a different name

— Reply to this email directly, view it on GitHub https://github.com/onepub-dev/dcli/pull/246#discussion_r1597515015, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAG32OFGJRCWBZIQ3TPIBUDZB2KMNAVCNFSM6AAAAABHBGPULGVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZDANJRGI3DGNBUGU . You are receiving this because you were mentioned.Message ID: @.***>

[tsavo-at-pieces]

I'm at the point that if you can get you code merged that we do a 4.0 release.

They are still a few dangling issue but I think we have the core paths that most people use working.

As such I'm inclined to not let perfect get in the way of good enough.

Thoughts?

Completely agree - hustling tonight to get it rockin and rollin!

Shall we target a 4.0.0 release say EOW?

[tsavo-at-pieces]

@bsutton turns out that I had created unit tests to ensure multiple isolates in one process can not enter the same lock so we are good to go there 🙌

Link to unit test for that specifically found here

[tsavo-at-pieces]

So I'm a little confused - the PR title is linux only but then you state that the underlying implementation works on Windows and Linux?

I assume when you say linux you are including macos as well?

Windows support is good to go on the underlying native_semaphores package now 🎉

All tests passing here ✅

[bsutton]

Great, eow works for me too.

On Sun, 12 May 2024, 11:20 am Tsavo Knott, @.***> wrote:

So I'm a little confused - the PR title is linux only but then you state that the underlying implementation works on Windows and Linux?

I assume when you say linux you are including macos as well?

Windows support is good to go on the underlying native_semaphores package now 🎉

All tests passing here https://github.com/open-runtime/native_semaphores/actions/runs/9047751220 ✅ Screenshot.2024-05-11.at.9.19.23.PM.png (view on web) https://github.com/onepub-dev/dcli/assets/102485237/f591fcd0-5d95-4103-8b5f-21a2a09d0273

— Reply to this email directly, view it on GitHub https://github.com/onepub-dev/dcli/pull/246#issuecomment-2106080412, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAG32OGHEGRVYGON3MST2KTZB27U5AVCNFSM6AAAAABHBGPULGVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDCMBWGA4DANBRGI . You are receiving this because you were mentioned.Message ID: @.***>

[tsavo-at-pieces]

Great, eow works for me too.

Legendary 😎

Just verified that NamedLocks also is g2g on windows!

Testing in DCLI now, PR on deck!

[bsutton]

So should I merge?

On Sun, 12 May 2024, 11:29 am Tsavo Knott, @.***> wrote:

Great, eow works for me too.

Legendary 😎

Just verified that NamedLocks also is g2g on windows! Screenshot.2024-05-11.at.9.28.28.PM.png (view on web) https://github.com/onepub-dev/dcli/assets/102485237/86f20354-db47-43e7-83a9-320237b969fc

Testing in DCLI now, PR on deck!

— Reply to this email directly, view it on GitHub https://github.com/onepub-dev/dcli/pull/246#issuecomment-2106081902, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAG32OHFNBHSXF2A5CQIUJLZB3AYJAVCNFSM6AAAAABHBGPULGVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDCMBWGA4DCOJQGI . You are receiving this because you were mentioned.Message ID: @.***>

[tsavo-at-pieces]

Let me rip some testing and final adjustments and we should be g2g in next couple of hours MAX. Will report back in 30 with DCLI specific testing results!

[tsavo-at-pieces]

NamedLocks are looking solid - this may be good to merge here!

See unit tests here:

I updated the internals of DCLI NamedLock.withLock() and have a unit test preserved from before that used withLock and I also added one which used Runtime NamedLock.guard() directly as well 🙌

[bsutton]

I'm going to target Wednesday for a release as I will have some spare time.

Will try to clean up a few more if the failing unit tests. These are mostly problems with the tests.