File lock implementation #13
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1 @@ | ||
| *.go text eol=lf |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,36 @@ | ||
| name: Build and Test | ||
|
|
||
| on: | ||
| pull_request: | ||
| types: [opened, reopened, synchronize] | ||
| push: | ||
| branches: [dev, main] | ||
|
|
||
| jobs: | ||
| build_test: | ||
| strategy: | ||
| matrix: | ||
| os: [macos-latest, ubuntu-latest, windows-latest] | ||
|
|
||
| runs-on: ${{matrix.os}} | ||
|
|
||
| steps: | ||
| - uses: actions/checkout@v3 | ||
|
|
||
| - uses: actions/setup-go@v4 | ||
| with: | ||
| go-version-file: "go.mod" | ||
|
|
||
| - name: Build | ||
| run: go build -x ./extensions/... | ||
|
|
||
| - name: Test | ||
| run: go test -race -v ./... | ||
|
|
||
| - name: Lint | ||
| # lint even when a previous step failed | ||
| if: ${{!cancelled()}} | ||
| uses: golangci/golangci-lint-action@v3 | ||
| with: | ||
| version: v1.52 | ||
| args: -v |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,9 @@ | ||
| linters: | ||
| enable: | ||
| - gocritic | ||
| - gofmt | ||
|
|
||
| linters-settings: | ||
| gocritic: | ||
| enabled-checks: | ||
| - evalOrder |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,93 @@ | ||
| // Copyright (c) Microsoft Corporation. All rights reserved. | ||
| // Licensed under the MIT License. See LICENSE in the project root for license information. | ||
|
|
||
| package lock | ||
|
|
||
| import ( | ||
| "context" | ||
| "errors" | ||
| "fmt" | ||
| "os" | ||
| "path/filepath" | ||
| "runtime" | ||
| "syscall" | ||
| "time" | ||
|
|
||
| "github.com/AzureAD/microsoft-authentication-extensions-for-go/extensions/internal/flock" | ||
| ) | ||
|
|
||
| // timeout lets tests set the default amount of time allowed to acquire the lock | ||
| var timeout = 5 * time.Second | ||
|
|
||
| // flocker helps tests fake flock | ||
| type flocker interface { | ||
| Fh() *os.File | ||
| Path() string | ||
| TryLockContext(context.Context, time.Duration) (bool, error) | ||
| Unlock() error | ||
| } | ||
|
|
||
| // Lock uses a file lock to coordinate access to resources shared with other processes. | ||
| // Callers are responsible for preventing races within a process. Lock applies advisory | ||
| // locks on Linux and macOS and is therefore unreliable on these platforms when several | ||
| // processes concurrently try to acquire the lock. | ||
| type Lock struct { | ||
| f flocker | ||
| retryDelay time.Duration | ||
| } | ||
|
|
||
| // New is the constructor for Lock. "p" is the path to the lock file. | ||
| func New(p string, retryDelay time.Duration) (*Lock, error) { | ||
| // ensure all dirs in the path exist before flock tries to create the file | ||
| err := os.MkdirAll(filepath.Dir(p), os.ModePerm) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| return &Lock{f: flock.New(p), retryDelay: retryDelay}, nil | ||
| } | ||
|
|
||
| // Lock acquires the file lock on behalf of the process. The behavior of concurrent | ||
| // and repeated calls is undefined. For example, Linux may or may not allow goroutines | ||
| // scheduled on different threads to hold the lock simultaneously. | ||
| func (l *Lock) Lock(ctx context.Context) error { | ||
| if _, hasDeadline := ctx.Deadline(); !hasDeadline { | ||
| var cancel context.CancelFunc | ||
| ctx, cancel = context.WithTimeout(ctx, timeout) | ||
| defer cancel() | ||
| } | ||
| for { | ||
| // flock opens the file before locking it and returns errors due to an existing | ||
| // lock or one acquired by another process after this process has opened the | ||
| // file. We ignore some errors here because in such cases we want to retry until | ||
| // the deadline. | ||
| locked, err := l.f.TryLockContext(ctx, l.retryDelay) | ||
| if err != nil { | ||
| if !(errors.Is(err, os.ErrPermission) || isWindowsSharingViolation(err)) { | ||
| return err | ||
| } | ||
| } else if locked { | ||
| if fh := l.f.Fh(); fh != nil { | ||
| s := fmt.Sprintf("{%d} {%s}", os.Getpid(), os.Args[0]) | ||
| _, _ = fh.WriteString(s) | ||
| } | ||
| return nil | ||
| } | ||
| } | ||
| } | ||
|
|
||
| // Unlock releases the lock and deletes the lock file. | ||
| func (l *Lock) Unlock() error { | ||
| err := l.f.Unlock() | ||
| if err == nil { | ||
| err = os.Remove(l.f.Path()) | ||
| } | ||
| // ignore errors caused by another process deleting the file or locking between the above Unlock and Remove | ||
| if errors.Is(err, os.ErrNotExist) || errors.Is(err, os.ErrPermission) || isWindowsSharingViolation(err) { | ||
| return nil | ||
| } | ||
| return err | ||
| } | ||
|
|
||
| func isWindowsSharingViolation(err error) bool { | ||
| return runtime.GOOS == "windows" && errors.Is(err, syscall.Errno(32)) | ||
| } | ||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,133 @@ | ||
| // Copyright (c) Microsoft Corporation. All rights reserved. | ||
| // Licensed under the MIT License. See LICENSE in the project root for license information. | ||
|
|
||
| package lock | ||
|
There was a problem hiding this comment. I had written a good test for this for the file lock here: https://github.com/AzureAD/microsoft-authentication-extensions-for-dotnet/blob/main/tests/Microsoft.Identity.Client.Extensions.Msal.UnitTests/CrossPlatLockTests.cs#L55 It creates a bunch of processes, each of them making changes to a file (which is locked). And then see if the file contents are not trash. https://github.com/AzureAD/microsoft-authentication-extensions-for-dotnet/blob/main/tests/Microsoft.Identity.Client.Extensions.Msal.UnitTests/CrossPlatLockTests.cs#L55 That said, this lock impl is just a wrapper over this |
||
|
|
||
| import ( | ||
| "bytes" | ||
| "context" | ||
| "errors" | ||
| "io" | ||
| "os" | ||
| "path/filepath" | ||
| "runtime" | ||
| "testing" | ||
| "time" | ||
|
|
||
| "github.com/stretchr/testify/require" | ||
| ) | ||
|
|
||
| var ctx = context.Background() | ||
|
|
||
| type fakeFlock struct { | ||
| err error | ||
| p string | ||
| } | ||
|
|
||
| func (f fakeFlock) Fh() *os.File { | ||
| fh, _ := os.Open(f.p) | ||
| return fh | ||
| } | ||
|
|
||
| func (f fakeFlock) Path() string { | ||
| return f.p | ||
| } | ||
|
|
||
| func (f fakeFlock) TryLockContext(context.Context, time.Duration) (bool, error) { | ||
| return f.err == nil, f.err | ||
| } | ||
|
|
||
| func (f fakeFlock) Unlock() error { | ||
| return f.err | ||
| } | ||
|
|
||
| func TestCreatesAndRemovesFile(t *testing.T) { | ||
| p := filepath.Join(t.TempDir(), "nonexistent", t.Name()) | ||
| lock, err := New(p, 0) | ||
| require.NoError(t, err) | ||
| require.NoFileExists(t, p) | ||
|
|
||
| err = lock.Lock(ctx) | ||
| require.NoError(t, err) | ||
| require.FileExists(t, p, "Lock didn't create the file") | ||
|
|
||
| buf := bytes.NewBuffer(nil) | ||
| _, err = io.Copy(buf, lock.f.Fh()) | ||
| require.NoError(t, err) | ||
| require.NotEmpty(t, buf, "Lock didn't write debug info to the locked file") | ||
|
|
||
| err = lock.Unlock() | ||
| require.NoError(t, err) | ||
| require.NoFileExists(t, p, "Unlock didn't remove the file") | ||
| } | ||
|
|
||
| func TestFileExists(t *testing.T) { | ||
| p := filepath.Join(t.TempDir(), t.Name()) | ||
| f, err := os.Create(p) | ||
| require.NoError(t, err) | ||
| data := "stuff" | ||
| _, err = f.WriteString(data) | ||
| require.NoError(t, err) | ||
| require.NoError(t, f.Close()) | ||
|
|
||
| // Lock should succeed when the file exists but isn't locked | ||
| lock, err := New(p, 0) | ||
| require.NoError(t, err) | ||
| err = lock.Lock(ctx) | ||
| require.NoError(t, err) | ||
|
|
||
| buf := bytes.NewBuffer(nil) | ||
| _, err = io.Copy(buf, lock.f.Fh()) | ||
| require.NoError(t, err) | ||
| require.NotEqual(t, data, buf, "Lock didn't write debug info to the locked file") | ||
|
|
||
| require.NoError(t, lock.Unlock()) | ||
| require.NoFileExists(t, p, "Unlock didn't remove the file") | ||
| } | ||
|
|
||
| func TestLockError(t *testing.T) { | ||
| p := filepath.Join(t.TempDir(), t.Name()) | ||
| lock, err := New(p, 0) | ||
| require.NoError(t, err) | ||
| expected := errors.New("expected") | ||
| lock.f = fakeFlock{err: expected} | ||
| require.Equal(t, lock.Lock(ctx), expected) | ||
| } | ||
|
|
||
| func TestLockTimeout(t *testing.T) { | ||
| p := filepath.Join(t.TempDir(), t.Name()) | ||
| a, err := New(p, 0) | ||
| require.NoError(t, err) | ||
| err = a.Lock(ctx) | ||
| require.NoError(t, err) | ||
|
|
||
| defer func(d time.Duration) { timeout = d }(timeout) | ||
| timeout = 0 | ||
| b, err := New(p, 0) | ||
| require.NoError(t, err) | ||
|
|
||
| err = b.Lock(ctx) | ||
| require.ErrorIs(t, err, context.DeadlineExceeded) | ||
|
|
||
| require.NoError(t, a.Unlock()) | ||
| } | ||
|
|
||
| func TestUnlockErrors(t *testing.T) { | ||
| p := filepath.Join(t.TempDir(), t.Name()) | ||
| lock, err := New(p, 0) | ||
| require.NoError(t, err) | ||
|
|
||
| err = lock.Lock(ctx) | ||
| require.NoError(t, err) | ||
| if runtime.GOOS != "windows" { | ||
| // Remove would fail on Windows because the file lock is mandatory there | ||
| require.NoError(t, os.Remove(p)) | ||
| } | ||
| // Unlock should return nil even when the lock file has been removed | ||
| require.NoError(t, lock.Unlock()) | ||
|
|
||
| expected := errors.New("it didn't work") | ||
| lock.f = fakeFlock{err: expected} | ||
| actual := lock.Unlock() | ||
| require.Equal(t, expected, actual) | ||
| } | ||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,16 @@ | ||
| module github.com/AzureAD/microsoft-authentication-extensions-for-go | ||
|
|
||
| go 1.18 | ||
|
|
||
| require ( | ||
| github.com/stretchr/testify v1.8.2 | ||
| gopkg.in/check.v1 v1.0.0-20201130134442-10cb98267c6c | ||
| ) | ||
|
|
||
| require ( | ||
| github.com/davecgh/go-spew v1.1.1 // indirect | ||
| github.com/kr/pretty v0.2.1 // indirect | ||
| github.com/kr/text v0.1.0 // indirect | ||
| github.com/pmezard/go-difflib v1.0.0 // indirect | ||
| gopkg.in/yaml.v3 v3.0.1 // indirect | ||
| ) |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,23 @@ | ||
| github.com/davecgh/go-spew v1.1.0/go.mod h1:J7Y8YcW2NihsgmVo/mv3lAwl/skON4iLHjSsI+c5H38= | ||
| github.com/davecgh/go-spew v1.1.1 h1:vj9j/u1bqnvCEfJOwUhtlOARqs3+rkHYY13jYWTU97c= | ||
| github.com/davecgh/go-spew v1.1.1/go.mod h1:J7Y8YcW2NihsgmVo/mv3lAwl/skON4iLHjSsI+c5H38= | ||
| github.com/kr/pretty v0.2.1 h1:Fmg33tUaq4/8ym9TJN1x7sLJnHVwhP33CNkpYV/7rwI= | ||
| github.com/kr/pretty v0.2.1/go.mod h1:ipq/a2n7PKx3OHsz4KJII5eveXtPO4qwEXGdVfWzfnI= | ||
| github.com/kr/pty v1.1.1/go.mod h1:pFQYn66WHrOpPYNljwOMqo10TkYh1fy3cYio2l3bCsQ= | ||
| github.com/kr/text v0.1.0 h1:45sCR5RtlFHMR4UwH9sdQ5TC8v0qDQCHnXt+kaKSTVE= | ||
| github.com/kr/text v0.1.0/go.mod h1:4Jbv+DJW3UT/LiOwJeYQe1efqtUx/iVham/4vfdArNI= | ||
| github.com/pmezard/go-difflib v1.0.0 h1:4DBwDE0NGyQoBHbLQYPwSUPoCMWR5BEzIk/f1lZbAQM= | ||
| github.com/pmezard/go-difflib v1.0.0/go.mod h1:iKH77koFhYxTK1pcRnkKkqfTogsbg7gZNVY4sRDYZ/4= | ||
| github.com/stretchr/objx v0.1.0/go.mod h1:HFkY916IF+rwdDfMAkV7OtwuqBVzrE8GR6GFx+wExME= | ||
| github.com/stretchr/objx v0.4.0/go.mod h1:YvHI0jy2hoMjB+UWwv71VJQ9isScKT/TqJzVSSt89Yw= | ||
| github.com/stretchr/objx v0.5.0/go.mod h1:Yh+to48EsGEfYuaHDzXPcE3xhTkx73EhmCGUpEOglKo= | ||
| github.com/stretchr/testify v1.7.1/go.mod h1:6Fq8oRcR53rry900zMqJjRRixrwX3KX962/h/Wwjteg= | ||
| github.com/stretchr/testify v1.8.0/go.mod h1:yNjHg4UonilssWZ8iaSj1OCr/vHnekPRkoO+kdMU+MU= | ||
| github.com/stretchr/testify v1.8.2 h1:+h33VjcLVPDHtOdpUCuF+7gSuG3yGIftsP1YvFihtJ8= | ||
| github.com/stretchr/testify v1.8.2/go.mod h1:w2LPCIKwWwSfY2zedu0+kehJoqGctiVI29o6fzry7u4= | ||
| gopkg.in/check.v1 v0.0.0-20161208181325-20d25e280405/go.mod h1:Co6ibVJAznAaIkqp8huTwlJQCZ016jof/cbN4VW5Yz0= | ||
| gopkg.in/check.v1 v1.0.0-20201130134442-10cb98267c6c h1:Hei/4ADfdWqJk1ZMxUNpqntNwaWcugrBjAiHlqqRiVk= | ||
| gopkg.in/check.v1 v1.0.0-20201130134442-10cb98267c6c/go.mod h1:JHkPIbrfpd72SG/EVd6muEfDQjcINNoR0C8j2r3qZ4Q= | ||
| gopkg.in/yaml.v3 v3.0.0-20200313102051-9f266ea9e77c/go.mod h1:K4uyk7z7BCEPqu6E+C64Yfv1cQ7kz7rIZviUmN+EgEM= | ||
| gopkg.in/yaml.v3 v3.0.1 h1:fxVm/GzAzEWqLHuvctI91KS9hhNmmWOoWu0XTYJS7CA= | ||
| gopkg.in/yaml.v3 v3.0.1/go.mod h1:K4uyk7z7BCEPqu6E+C64Yfv1cQ7kz7rIZviUmN+EgEM= |
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like there's some brittleness on certain platforms. Do you think it would be worth it to add some kind of internal counter (or whatever) and return an error (or potentially
panic()) if the invariant isn't met (e.g. concurrent calls toLock())?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It wouldn't be worth a panic because two goroutines holding the lock simultaneously manifests as a cache miss. If the goroutines write the cache sequentially, one overwrites the other's data i.e., one token is lost. If their timing is extremely unlucky and the storage backend allows it, they could make overlapping writes that corrupt the whole cache, losing all its tokens. That's non-fatal because the application can reauthenticate. I suppose an application could lose many tokens, exceed the STS's rate limit with a deluge of token requests and get throttled, which would be tough to recover from, but that requires the alignment of many evil stars.
I think we're okay here. Customers will use this type indirectly through
Cache(#14), which synchronizes goroutines. If applications use only oneCacheper storage location (we'll document that they should), that eliminates the risk. Off the top of my head I can't think of a good way to catch multiple instances accessing the same storage, but trying doesn't seem worth the effort to me. Anyway, that's my reasoning, please let me know if I'm neglecting something.