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

output: make plugin context operations safe for concurrent use #46

Merged
merged 1 commit into from
Dec 13, 2021

Conversation

l2dy
Copy link
Contributor

@l2dy l2dy commented Aug 5, 2021

See #45.

@nokute78
Copy link
Contributor

nokute78 commented Aug 6, 2021

How about using a plane map + RWMutex ?

https://pkg.go.dev/sync#Map

The Map type is specialized. Most code should use a plain Go map instead,
 with separate locking or coordination, for better type safety and to make it easier to maintain other invariants along with the map content.

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.

@l2dy
Copy link
Contributor Author

l2dy commented Aug 6, 2021

Output plugins should be heavy on reading the contexts map, not updating it. If context state update is needed in FLBPluginFlushCtx, store a pointer to the mutable state in context and it will be out of the scope of this sync.Map.

Only FLBPluginInit has access to the plugin struct, and every call assigns a new key.

@l2dy
Copy link
Contributor Author

l2dy commented Aug 6, 2021

In fact, every time FLBPluginSetContext is called a new key is used, so this is exactly use case (1), a cache that only grows.

@nokute78
Copy link
Contributor

nokute78 commented Aug 6, 2021

OK.
I understood FLBPluginSetContext doesn't support overwriting.
Thank you.

@l2dy
Copy link
Contributor Author

l2dy commented Oct 14, 2021

@edsiper Could you review this?

@l2dy
Copy link
Contributor Author

l2dy commented Dec 12, 2021

@nokute78 Can you merge this pull request?

@nokute78
Copy link
Contributor

nokute78 commented Dec 12, 2021

@l2dy @edsiper
Sorry I can't merge since I don't have a permission to merge for this repo.

This PR is LGTM.

@edsiper edsiper merged commit 0be1ffb into fluent:master Dec 13, 2021
@edsiper
Copy link
Member

edsiper commented Dec 13, 2021

@nokute78 pls check for permissions now...

@l2dy l2dy deleted the sync-map branch December 13, 2021 02:51
@nokute78
Copy link
Contributor

Oh I can find "Merge pull request" button now.

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.

3 participants