Skip to content

Add mutex to sources map#25

Merged
meeehow merged 2 commits intogoogle:mainfrom
ZetaTwo:issue-21
Nov 25, 2022
Merged

Add mutex to sources map#25
meeehow merged 2 commits intogoogle:mainfrom
ZetaTwo:issue-21

Conversation

@ZetaTwo
Copy link
Contributor

@ZetaTwo ZetaTwo commented Nov 19, 2022

Looking at the logs in #21 it seems that the problematic access is hashr.go:324. This PR adds a mutex to that access. I don't know if this really is the best way to solve this but with this patch I am no longer seeing the crash.

@meeehow
Copy link
Contributor

meeehow commented Nov 21, 2022

I think there are two ways to solve this issue:

  1. What you proposed, however in this case we should wrap every assignment to h.processingSources map with h.processingSourcesMutex.Lock()/h.processingSourcesMutex.Unlock()
  2. Use sync.Map instead of regular map to store the data

I think 2. might be a cleaner approach here

wg sync.WaitGroup
mu sync.Mutex
processingSources map[string]*ProcessingSource
processingSourcesMutex sync.RWMutex
Copy link
Contributor

Choose a reason for hiding this comment

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

PTAL at my comment in PR

@ZetaTwo
Copy link
Contributor Author

ZetaTwo commented Nov 21, 2022

That line is the only place where we create new entries in the map. All other places just assign to properties of an entry in the map. Would a sync.Map make any difference in that case?

@meeehow
Copy link
Contributor

meeehow commented Nov 24, 2022

From a low level perspective there should be no difference between creating new entry and modifying existing one, in both cases we are writing to a map. In Go concurrent writes or writes/reads are not thread safe, only concurrent reading is thread safe.

Having sync.Map is one way of making the whole process thread safe.

@ZetaTwo
Copy link
Contributor Author

ZetaTwo commented Nov 24, 2022

Isn't there a difference between writing to the Map structure itself, i.e. creating or deleting records and modifying the data of those records?

Regardless, sync.Map is probably the right way to go judging from this paragraph from the documentation:

The Map type is optimized for two common use cases: (1) when the entry for a given key is only ever written once but
read many times, as in caches that only grow, or (2) when multiple goroutines read, write, and overwrite entries for disjoint sets of keys. In these two cases, use of a Map may significantly reduce lock contention compared to a Go map paired with a separate Mutex or RWMutex.

I can update the PR to use that instead.

@ZetaTwo
Copy link
Contributor Author

ZetaTwo commented Nov 24, 2022

Actually, nvm. Dropping the type safety seems like a terrible idea. I instead went ahead and added proper read locks to the rest of the code.

To clarify my point above:

map[key] = entry

will be a write to the map but

map[key].property = value

is just a read in the map but then modifying the contents of the entry.

@meeehow
Copy link
Contributor

meeehow commented Nov 25, 2022

Thanks for fixing this!

@meeehow meeehow merged commit 0c64ef3 into google:main Nov 25, 2022
@ZetaTwo ZetaTwo deleted the issue-21 branch November 30, 2022 21:52
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