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

Fix stale lockfile detection #3687

Merged
merged 1 commit into from
Nov 2, 2022

Conversation

HebaruSan
Copy link
Member

@HebaruSan HebaruSan commented Oct 28, 2022

Background

CKAN creates a <KSP>/CKAN/registry.locked file when it loads an instance and deletes this file at exit. If CKAN terminates abnormally (e.g., pkill -KILL mono), then the registry.locked file can be left behind without a running CKAN. To make this condition detectable, the file contains the process ID of the CKAN process that created it.

Problem

If you have a stale lock file and you start CKAN, the staleness is not detected. The lock file dialog appears even though there is no process with the process ID contained in the lock file.

Cause

#2139 was supposed to address this, but it had a bug:

if (IsInstanceMaybeLocked(path))

That function takes the <KSP>/CKAN dir path as a parameter:

private static string InstanceRegistryLockPath(string path)
{
return Path.Combine(path, "registry.locked");
}
public static bool IsInstanceMaybeLocked(string path)
{
return File.Exists(InstanceRegistryLockPath(path));
}

... but we're passing the path to the registry.json file, so the file path that we look for is <KSP>/CKAN/registry.json/registry.locked:

this.path = Path.Combine(path, "registry.json");

GameInstance.cs uses it correctly:

/// <returns>
/// true if the instance may be locked, false otherwise.
/// Note that this is a tentative value; if it's true,
/// we still need to try to acquire the lock to confirm it isn't stale.
/// NOTE: Will throw NotKSPDirKraken if the instance isn't valid!
/// Either be prepared to catch that exception, or check Valid first to avoid it.
/// </returns>
public bool IsMaybeLocked => RegistryManager.IsInstanceMaybeLocked(CkanDir());

Changes

  • Now CheckStaleLock passes the path to the CKAN dir as intended, so the lock file deletion takes place
    1392 [1] DEBUG CKAN.RegistryManager (null) - Preparing to load registry at /home/user/.local/share/Steam/steamapps/common/Kerbal Space Program/CKAN
    1393 [1] DEBUG CKAN.RegistryManager (null) - Checking for stale lock file at /home/user/.local/share/Steam/steamapps/common/Kerbal Space Program/CKAN/registry.locked
    1393 [1] DEBUG CKAN.RegistryManager (null) - Lock file found at /home/user/.local/share/Steam/steamapps/common/Kerbal Space Program/CKAN/registry.locked
    1393 [1] DEBUG CKAN.RegistryManager (null) - Lock file contents: 122202
    1393 [1] DEBUG CKAN.RegistryManager (null) - Looking for process with ID: 122202
    1394 [1] DEBUG CKAN.RegistryManager (null) - Deleting stale lock file at /home/user/.local/share/Steam/steamapps/common/Kerbal Space Program/CKAN/registry.locked
    1394 [1] DEBUG CKAN.RegistryManager (null) - Trying to create lock file: /home/user/.local/share/Steam/steamapps/common/Kerbal Space Program/CKAN/registry.locked
    1394 [1] DEBUG CKAN.RegistryManager (null) - Lock file created: /home/user/.local/share/Steam/steamapps/common/Kerbal Space Program/CKAN/registry.locked
    1395 [1] DEBUG CKAN.RegistryManager (null) - Trying to load registry from /home/user/.local/share/Steam/steamapps/common/Kerbal Space Program/CKAN/registry.json
    
  • RegistryManager.ksp is renamed gameInstance
  • The parameters of InstanceRegistryLockPath and IsInstanceMaybeLocked are renamed from path to ckanDirPath to make it easier to know the correct thing to pass

Fixes #3686.

@HebaruSan HebaruSan added Bug Something is not working as intended Easy This is easy to fix Core (ckan.dll) Issues affecting the core part of CKAN Pull request Linux Issues specific for Linux Mono Issues specific for Mono labels Oct 28, 2022
Copy link
Member

@techman83 techman83 left a comment

Choose a reason for hiding this comment

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

Yup, that makes sense. 🎉

@HebaruSan HebaruSan merged commit 5a73b80 into KSP-CKAN:master Nov 2, 2022
@HebaruSan HebaruSan deleted the fix/stale-lockfile branch November 2, 2022 01:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Something is not working as intended Core (ckan.dll) Issues affecting the core part of CKAN Easy This is easy to fix Linux Issues specific for Linux Mono Issues specific for Mono
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Bug] CKAN already running dialog appears even if PID in registry.locked is stale
2 participants