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 file lock issue on windows #204

Merged
merged 1 commit into from
Aug 8, 2024

Conversation

anujc25
Copy link
Contributor

@anujc25 anujc25 commented Aug 7, 2024

What this PR does / why we need it

Background:

  • As part of Update the file locking module dependency #200 we switched to use github.com/alexflint/go-filemutex package instead of github.com/juju/fslock.
  • The Unlock function between the two libraries behaves differently. The main difference is the go-filemutex.Unlock does not close the file whereas the fslock.Unlock does close the file, automatically releasing any acquired lock.
  • Because of the above difference we noticed that when the following sequence of operations are performed the acquiring the lock fails because the file was kept open by the first lock call.
    • go-filemutex.Lock
    • go-filemutex.Unlock
    • fslock.Lock
  • The above behavior can be noticed when the Tanzu CLI uses go-filemutex to lock the file using newer version of tanzu-plugin-runtime and plugins use fslock to lock the file using an older version of tanzu-plugin-runtime

Fix:

  • Instead of using go-filemutex.Unlock use go-filemutex.Close to unlock the file and close the file descriptor. Doing this will make the newer version of library using go-filemutex compatible with older version of library which uses fslock

Which issue(s) this PR fixes

Fixes #

Describe testing done for PR

  • Manual tests using a custom code that uses both libraries together.
// Copyright 2022 VMware, Inc. All Rights Reserved.
// SPDX-License-Identifier: Apache-2.0

package main

import (
	"os"

	"github.com/anujc/test-locking/filemutex"
	"github.com/anujc/test-locking/juju"
)

func main() {
	if len(os.Args) < 2 {
		panic("argument is required. Values: '1' or '2'")
	}

	arg := os.Args[1]
	if arg == "1" {
		filemutex.AcquireLock()
		filemutex.ReleaseLock()

		juju.AcquireLock()
		juju.ReleaseLock()

	} else if arg == "2" {
		juju.AcquireLock()
		juju.ReleaseLock()

		filemutex.AcquireLock()
		filemutex.ReleaseLock()
	} else {
		panic("incorrect argument")
	}
}
  • Without the fix the above test program results are as follows:

    • Mac & Linux:
      • 1 -> Success
      • 2 -> Success
    • Windows:
      • 1 -> Failed
      • 2 -> Success
  • With the fix the above test program results are as follows:

    • Mac & Linux:
      • 1 -> Success
      • 2 -> Success
    • Windows:
      • 1 -> Success
      • 2 -> Success

Release note

Fix file lock issue on windows

Additional information

Special notes for your reviewer

@anujc25 anujc25 requested a review from a team as a code owner August 7, 2024 22:04
Copy link
Contributor

@marckhouzam marckhouzam left a comment

Choose a reason for hiding this comment

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

Awesome! Thank you for this!

Copy link
Contributor

@vuil vuil left a comment

Choose a reason for hiding this comment

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

Thanks for the fix!

@anujc25 anujc25 merged commit de610f9 into vmware-tanzu:main Aug 8, 2024
5 checks passed
anujc25 added a commit to anujc25/tanzu-plugin-runtime that referenced this pull request Aug 8, 2024
@marckhouzam marckhouzam added this to the v1.4.2 milestone Aug 8, 2024
anujc25 added a commit that referenced this pull request Aug 8, 2024
anujc25 added a commit to anujc25/tanzu-cli that referenced this pull request Aug 8, 2024
anujc25 added a commit to anujc25/tanzu-cli that referenced this pull request Aug 8, 2024
anujc25 added a commit to vmware-tanzu/tanzu-cli that referenced this pull request Aug 9, 2024
anujc25 added a commit to anujc25/tanzu-cli that referenced this pull request Aug 14, 2024
anujc25 added a commit to vmware-tanzu/tanzu-cli that referenced this pull request Aug 14, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants