Skip to content

Conversation

@abhidnya13
Copy link
Contributor

No description provided.

"github.com/AzureAD/microsoft-authentication-extensions-for-go/flock"
)

type CrossPlatLock struct {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

type Lock will be better
Lock.New() for initializing Lock
Directory structure - extensions main directory and then lock directory

)

type CrossPlatLock struct {
retryNumber int

Choose a reason for hiding this comment

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

Rename "retries"


type CrossPlatLock struct {
retryNumber int
retryDelayMilliseconds time.Duration

Choose a reason for hiding this comment

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

rename retryDelay


lockFile *os.File

fileLock *flock.Flock

Choose a reason for hiding this comment

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

fLock


fileLock *flock.Flock

lockfileName string

Choose a reason for hiding this comment

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

Not used.

fileLock *flock.Flock

lockfileName string
locked bool

Choose a reason for hiding this comment

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

Not used.


lockfileName string
locked bool
}

Choose a reason for hiding this comment

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

Add internal sync.Mutex called mu.

}

func (c CrossPlatLock) Lock() error {
for tryCount := 0; tryCount < c.retryNumber; tryCount++ {

Choose a reason for hiding this comment

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

Add these lines:

c.mu.Lock()
defer c.mu.Unlock()

Change (c CrossPlatLock) to use a pointer

locked bool
}

func NewLock(lockFileName string, retryNumber int, retryDelayMilliseconds time.Duration) (CrossPlatLock, error) {

Choose a reason for hiding this comment

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

Have this return a pointer

Choose a reason for hiding this comment

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

Let's get rid of lockFileName, use a default defined in a constructor.

Let's use this signature:

type Option func(l *Lock)

func WithRetries(n int) Option{
return func(l *Lock) {
l.retries = n
}
}

func New(options ...Option) (*Lock, error) {
l := &Lock{
...
}
for _, o := range options {
o(l)
}
...
return l, nil
}

return errors.New("Failed to acquire lock")
}

func (c CrossPlatLock) UnLock() error {

Choose a reason for hiding this comment

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

Add defer c.mu.Unlock()

return nil
}
}
return errors.New("Failed to acquire lock")

Choose a reason for hiding this comment

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

error text must start with lower case letter


func validateResult(cacheFile string, t *testing.T) int {
count := 0
var prevProc string = ""
Copy link
Contributor Author

Choose a reason for hiding this comment

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

var
(
count int
prevProc, tag, proc string
)
)

if err != nil {
log.Println(err)
}
dat := string(data)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

All in one line and call it s instead of ele

}

func New(lockFileName string, options ...Option) (*Lock, error) {
l := &Lock{}

Choose a reason for hiding this comment

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

You need to add default value for retries and retryDelay.

l.retries = n
}
}
func WithRetryDelay(t time.Duration) Option {

Choose a reason for hiding this comment

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

Public requires a comment. I'd put:

// WithRetryDelay changes the default delay from [delay] to t.


type Option func(l *Lock)

func WithRetries(n int) Option {

Choose a reason for hiding this comment

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

Public requires a comment. I'd put:

// WithRetries changes the default number of retries from [retries] to n. Negative values panic()

Also, add panic on negative values.

}
}

func New(lockFileName string, options ...Option) (*Lock, error) {

Choose a reason for hiding this comment

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

lockFileName can be just "p".

// New creates a Lock for a lock file at "p". If "p" doesn't exist, it will be created when using. Lock().
func New(p string options ...Option) (*Lock, error)

func (l *Lock) Lock() error {
l.mu.Lock()
defer l.mu.Unlock()
for tryCount := 0; tryCount < l.retries; tryCount++ {

Choose a reason for hiding this comment

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

for i := 0; i < l.retries; i++ {
...

prevProc = ""
}
return count, nil
}

Choose a reason for hiding this comment

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

Add two tests:

One tests all the conditions of Lock().
Another tests all the conditions of Unlock()

  • flock.Unlock() has error
  • Lockfile can't be removed

You can do this by providing an interface:

type flocker interface{
TryLock() error
Unlock() error
Path() string
}

type fakeFlock struct {
err bool
}

func (f fakeFlock) TryLock() error {
if f.err {
return errors.New("error")
}
return nil
}

func (f fakeFlock) Unlock() error {
if f.err {
return errors.New("error")
}
return nil
}

const cacheFile = "cache.txt"

func TestLocking(t *testing.T) {

Choose a reason for hiding this comment

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

Remove blank line

if strings.TrimSpace(s) == "" {
continue
}
count += 1

Choose a reason for hiding this comment

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

count++

require (
github.com/AzureAD/microsoft-authentication-library-for-go v0.3.0
github.com/billgraziano/dpapi v0.4.0
github.com/gofrs/flock v0.8.1

Choose a reason for hiding this comment

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

This flock line shouldn't. be here. So either a path change is needed or you need to run go tidy

@@ -0,0 +1,50 @@
package internal

Choose a reason for hiding this comment

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

Move these files under:

internal/accessor
accessor.go
file/
file.go
windows/
window.go

accessor.go:

  • Defines type Cache interface{}
    file.go:
  • Defines Accessor struct{}
  • Defines New() constructor
    windows.go
  • Defines Accessor struct{}
  • Defines New() constructor

All "Accessor" types implement accessor.Cache

@chlowell
Copy link
Contributor

chlowell commented Jun 8, 2023

Superseded by #13 and #15

@chlowell chlowell closed this Jun 8, 2023
@chlowell chlowell deleted the windows_encryption branch June 8, 2023 16:39
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.

4 participants