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 cache race #4

Merged
merged 3 commits into from
May 19, 2021
Merged

Fix cache race #4

merged 3 commits into from
May 19, 2021

Conversation

aschmahmann
Copy link
Contributor

No description provided.

sync.RWMutex
mx sync.Mutex
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Figured exporting the Lock/Unlock functions here was unintentional and a bad idea for a consumer to use.

Copy link
Collaborator

Choose a reason for hiding this comment

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

ah yes, it was just programmatic convenience!

@aschmahmann aschmahmann requested a review from vyzo May 19, 2021 17:24
@@ -101,8 +101,8 @@ func (r *Resolver) LookupTXT(ctx context.Context, domain string) ([]string, erro
}

func (r *Resolver) getCachedIPAddr(domain string) ([]net.IPAddr, bool) {
r.RLock()
defer r.RUnlock()
r.mx.Lock()
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Switching away from RW mutexes may slow things down here. Given that we're caching against what would be a network request waiting on a mutex doesn't feel too bad, and if this becomes an issue we can always come back and optimize it.

Copy link
Collaborator

Choose a reason for hiding this comment

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

let's add a follow up issue? It shouldn't be hard to do, just queue the entries for cleanup and do that with write lock.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

#5

sync.RWMutex
mx sync.Mutex
Copy link
Collaborator

Choose a reason for hiding this comment

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

ah yes, it was just programmatic convenience!

@@ -101,8 +101,8 @@ func (r *Resolver) LookupTXT(ctx context.Context, domain string) ([]string, erro
}

func (r *Resolver) getCachedIPAddr(domain string) ([]net.IPAddr, bool) {
r.RLock()
defer r.RUnlock()
r.mx.Lock()
Copy link
Collaborator

Choose a reason for hiding this comment

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

let's add a follow up issue? It shouldn't be hard to do, just queue the entries for cleanup and do that with write lock.

@vyzo vyzo merged commit 343363c into master May 19, 2021
@vyzo vyzo deleted the fix/cache-race branch May 19, 2021 17:34
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.

2 participants