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

Add directory locking mechanism #500

Merged
merged 5 commits into from
Jan 18, 2024
Merged

Add directory locking mechanism #500

merged 5 commits into from
Jan 18, 2024

Conversation

webmaster128
Copy link
Member

@webmaster128 webmaster128 commented Jan 17, 2024

In the past we have seen cases in which multiple instaces ran on the same directory in parallel (especially test setups), leading to unreliable and hard to debug issues down the road.

  • Implementation
  • Add MIGRATING entry

Copy link
Collaborator

@chipshort chipshort left a comment

Choose a reason for hiding this comment

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

LGTM
Bit unfortunate that this is Unix-only, but I guess we can always add Windows support later if we continue working on that.

Comment on lines 84 to 87
_ = syscall.Flock(int(cache.lockfile.Fd()), syscall.LOCK_UN)
cache.lockfile.Close()
Copy link
Collaborator

Choose a reason for hiding this comment

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

Closing the file releases the lock anyways, so no need for the explicit syscall

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh, are you sure? I found it this way in some example. Don't know. Should we remove the unlock?

Copy link
Collaborator

Choose a reason for hiding this comment

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

The linux manpage for flock mentions:

[...] the lock is released either by an explicit LOCK_UN operation on any of these duplicate descriptors, or when all such descriptors have been closed.

For bsd, it's described in the page about close:

on the last close of a file holding an advisory lock the lock is released

Whether to remove it or not is probably more of a taste question, since this is not a hot path (In fact, as far as I can see, wasmd never calls this).

Copy link
Collaborator

Choose a reason for hiding this comment

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

and on my Macbook, the TestInitLockingPreventsConcurrentAccess test still passes if I remove the syscall

Copy link
Member Author

Choose a reason for hiding this comment

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

I'll remove it and just add a comment that closing the file releases the lock for simplicity

@webmaster128
Copy link
Member Author

Bit unfortunate that this is Unix-only, but I guess we can always add Windows support later if we continue working on that.

Oh, interesting. Do you have more information we can add ass a comment or something? I was assuming this is a wrapper around some system specific locking. The Rust one has some sort of Windows support too.

@chipshort
Copy link
Collaborator

I don't know about file locking specifically, but the Rust stdlib tries to provide cross-platform wrappers wherever possible and all platform specific stuff is clearly marked in corresponding packages.
This Go package however is completely platform-specific. It is not mentioned very clearly in the syscall module docs, but if you change the platform in the dropdown on the right to windows, you get completely different functions.
Also, now that I look at it more closely, the docs state:

Deprecated: this package is locked down. Callers should use the corresponding package in the golang.org/x/sys repository instead. That is also where updates required by new systems or versions should be applied. See https://golang.org/s/go1.4-syscall for more information.

So, maybe we should use sys/unix instead?
If we want cross-platform support, there also seem to be some packages that provide such a wrapper, e.g. https://pkg.go.dev/github.com/gofrs/flock

@webmaster128
Copy link
Member Author

Oh that's great insights. I was not aware that those different APIs per target platform exist. Reading through https://go.googlesource.com/proposal/+/refs/heads/master/design/freeze-syscall.md makes me think using golang.org/x/sys/unix is a good approach as it makes explicit what we do here. We can build a cross-platform solution ourselves if needed one day. But right now all of internal/api cannot work on Windows since it requires a working libwasmvm build which we don't have.

Copy link
Collaborator

@chipshort chipshort left a comment

Choose a reason for hiding this comment

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

Looks good 👍

@webmaster128 webmaster128 merged commit c28a904 into main Jan 18, 2024
13 checks passed
@webmaster128 webmaster128 deleted the lockfile branch January 18, 2024 11:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants