Skip to content
This repository has been archived by the owner on Jun 30, 2023. It is now read-only.

Fix cache notification locking semantics [1571] #1572

Merged
merged 1 commit into from
Apr 4, 2019
Merged

Fix cache notification locking semantics [1571] #1572

merged 1 commit into from
Apr 4, 2019

Conversation

MarkZuber
Copy link
Contributor

jennyf19
jennyf19 approved these changes Apr 3, 2019
View reviewed changes
Copy link
Contributor

@jennyf19 jennyf19 left a comment

Choose a reason for hiding this comment

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

LGTM

bgavrilMS
bgavrilMS approved these changes Apr 3, 2019
View reviewed changes
Copy link
Member

@bgavrilMS bgavrilMS left a comment

Choose a reason for hiding this comment

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

I don't see the whole code, but as long we don't double notify (i.e. notify during a notification), LGTM

@MarkZuber MarkZuber merged commit d9e2732 into dev Apr 4, 2019
@MarkZuber MarkZuber deleted the markzuber/cachenotify branch April 4, 2019 02:32
@jmprieur
Copy link
Contributor

jmprieur commented Apr 4, 2019

which problem is it solving?

@bgavrilMS
Copy link
Member

@jmprieur The current implementation is causing deadlocks if the token cache file is syncronized from the outside. Check out the bug #1571 to see a deadlock.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@jennyf19 jennyf19 jennyf19 approved these changes

@bgavrilMS bgavrilMS bgavrilMS approved these changes

@jmprieur jmprieur Awaiting requested review from jmprieur

@trwalke trwalke Awaiting requested review from trwalke

@henrik-me henrik-me Awaiting requested review from henrik-me

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@MarkZuber @jmprieur @bgavrilMS @jennyf19