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

Use system wide file locks to synchronize log files. #1605

Merged
merged 1 commit into from
Aug 7, 2024

Conversation

Hoikas
Copy link
Member

@Hoikas Hoikas commented Jul 20, 2024

This is an alternative to #1603, inspired by #1603 (comment).

The global semaphore has been observed to not be released on unix like platforms when the process exits. So, we convert to file locks, which will be released on process exit on unix like platforms. Unfortunately, Win32 file locks and POSIX file locks have slightly different semantics in that Win32 locks are mandatory and POSIX locks are advisory, which can result in slightly different behavior. OTOH, this should fix the client deadlocking until the system is restarted, so this should be an overall win.

The global semaphore has been observed to not be released on unix like
platforms when the process exits. So, we convert to file locks, which
will be released on process exit on unix like platforms. Unfortunately,
Win32 file locks and POSIX file locks have slightly different semantics
in that Win32 locks are mandatory and POSIX locks are advisory, which
can result in slightly different behavior. OTOH, this should fix the
client deadlocking until the system is restarted, so this should be an
overall win.
Copy link
Member

@dpogue dpogue left a comment

Choose a reason for hiding this comment

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

This looks reasonable to me.

I don't like the hsFILELock name (would prefer hsFileLock), but it locks a FILE* so I guess it's accurate. We prefer this in CoreLib rather than PubUtilLib/plFile?

@colincornaby
Copy link
Contributor

Has this been tested on macOS yet? It's implied that file locks should automatically release on macOS - but I didn't check myself.

@Hoikas
Copy link
Member Author

Hoikas commented Jul 22, 2024

Has this been tested on macOS yet? It's implied that file locks should automatically release on macOS - but I didn't check myself.

I tested it on my mac mini insofar as making sure that running multiple clients did not break in surprising ways. I haven't done any testing where the client crashes while the lock is being held, yet.

@colincornaby
Copy link
Contributor

Not a huge surprise - but I validated that macOS will release the file locks if the process is unexpectedly terminated.

@Hoikas
Copy link
Member Author

Hoikas commented Jul 26, 2024

Not a huge surprise - but I validated that macOS will release the file locks if the process is unexpectedly terminated.

Thanks! I've been forgetting to fire up the Mac and verify that. Did you also perchance check to see if the client deadlocks without this?

@colincornaby
Copy link
Contributor

Not a huge surprise - but I validated that macOS will release the file locks if the process is unexpectedly terminated.

Thanks! I've been forgetting to fire up the Mac and verify that. Did you also perchance check to see if the client deadlocks without this?

I've had it deadlock before and have had to manually delete the lock file that was created. My assumption had been the Windows client had the same issue - until I read the source.

@Hoikas
Copy link
Member Author

Hoikas commented Aug 6, 2024

Oh, this is still open. Any objections to this being merged?

@colincornaby
Copy link
Contributor

Nothing from me

@dpogue dpogue merged commit 13ffa2d into H-uru:master Aug 7, 2024
17 checks passed
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.

3 participants