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

Unify File Locking Across Domains #251

Closed
2 of 4 tasks
emmacasolin opened this issue Oct 20, 2021 · 4 comments
Closed
2 of 4 tasks

Unify File Locking Across Domains #251

emmacasolin opened this issue Oct 20, 2021 · 4 comments
Assignees
Labels
development Standard development enhancement New feature or request wontfix This will not be worked on

Comments

@emmacasolin
Copy link
Contributor

emmacasolin commented Oct 20, 2021

Specification

Previously we were using the proper-lockfile library to lock and unlock the session file during CLI calls, however, we have recently switched over to using the much simpler (and newer) fd-lock for this purpose. However, proper-lockfile is still being used for the agent lockfile, so we should change this to use fd-lock instead in order to unify our file locking across all domains.

Since fd-lock currently only has a lock() and unlock() command (and no way of checking if a file is locked without attempting to lock it or blocking access to a locked file) we will most likely need to look into the Node C++ bindings and the POSIX manual in order to adapt the library to our needs.

Additional context

See #236, where fd-lock was incorporated into the sessions domain.

Tasks

  • Look into the Node C++ bindings and POSIX manual
  • Make changes to the fd-lock library to meet our requirements
  • Replace proper-lockfile usage with fd-lock in the agent lockfile
  • Test that these changes have not affected functionality in any way
@emmacasolin emmacasolin added development Standard development enhancement New feature or request labels Oct 20, 2021
@CMCDragonkai
Copy link
Member

Instead of C++, one can also use Rust: https://github.com/napi-rs/napi-rs

Can be useful if you want to gain experience in this new language.

@CMCDragonkai
Copy link
Member

Locking at https://github.com/mafintosh/fd-lock/blob/master/binding.cc C++ code I see it using flock on linux/posix systems. The flock call actually supports read write locks, which means during reads of the session token file we should be using a shared lock, however this is not currently exposed in JS. The C++ code requires some changes to enable this and exposing shared vs exclusive locks. https://man7.org/linux/man-pages/man2/flock.2.html this shows that there's LOCK_SH flag capable of being used.

On win32 however, it should be swapped to using this call instead: https://docs.microsoft.com/en-us/windows/win32/api/fileapi/nf-fileapi-lockfileex

Also important would be to apply promise-based API, so one can wait or block on the locks as well. Promise based system would require the C++ to notify the events. It appears the Node API can be called from C++ to create a promise: https://github.com/nodejs/node-addon-api/blob/main/doc/promises.md

@CMCDragonkai
Copy link
Member

Testing should make use of a Windows CI/CD environment, just do manual test for now as it seems integrating Windows in our current CI/CD setup might be a bit difficult.

@CMCDragonkai CMCDragonkai added epic Big issue with multiple subissues and removed epic Big issue with multiple subissues labels Nov 5, 2021
@CMCDragonkai
Copy link
Member

No longer necessary because both Status and Session can just use fd-lock for now.

The Session is implemented in https://gitlab.com/MatrixAI/Engineering/Polykey/js-polykey/-/merge_requests/213

And Status is in #283.

This means lockfile domain and also the dependency proper-lockfile can be removed from js-polykey.

In the future we can improve lock performance and lock portability by following the above instructions/notes, but for now it's a wontfix.

@tegefaulkes @emmacasolin @joshuakarp

Note that this only applies to "file locking". Which is only used by Status and Session. Currently all other uses of locking uses async-mutex which is for in-process locking, not inter-process locking. And we already have a RWLock implementation on top of async-mutex.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
development Standard development enhancement New feature or request wontfix This will not be worked on
Development

No branches or pull requests

3 participants