Encapsulation for Go plugin instances [sc-141627] #110
Conversation
|
This change is part of the following stack: Change managed by git-spice. |
alecholmes
force-pushed
the
alec/2025-07-08-encapsulate-plugin-instance
branch
from
d9607df to
beb6f18
Compare
| // also, the interfaces for input and output plugins. | ||
| package plugin | ||
|
|
||
| import "C" |
There was a problem hiding this comment.
This is coming in from C.size_t use here:
func (p *pluginInstance) callback(data *unsafe.Pointer, csize *C.size_t) int {
If there's some reason we want to contain CGO to cshared.go, that's doable.
| "github.com/calyptia/plugin/metric" | ||
| ) | ||
|
|
||
| var ( |
There was a problem hiding this comment.
All the globals are in this var block.
alecholmes
force-pushed
the
alec/2025-07-08-encapsulate-plugin-instance
branch
2 times, most recently
from
4e14c14 to
f02efa2
Compare
alecholmes
changed the title
alecholmes
force-pushed
the
alec/2025-07-08-encapsulate-plugin-instance
branch
from
d641376 to
5add7d9
Compare
alecholmes
force-pushed
the
alec/2025-07-08-encapsulate-plugin-instance
branch
from
cbb2a94 to
06cf1dc
Compare
| return input.FLB_ERROR | ||
| } | ||
|
|
||
| func cleanup() int { |
There was a problem hiding this comment.
Global cleanup logic moved into FLBPluginExit, which was previously calling this.
stoksc
left a comment
stoksc
left a comment
There was a problem hiding this comment.
this looks really good, nice. all questions are just for my own learning.
| initWG.Add(1) | ||
| defer initWG.Done() | ||
| func FLBPluginInit(ptr unsafe.Pointer) (respCode int) { | ||
| currInstanceMu.Lock() |
There was a problem hiding this comment.
just so i'm following, currInstanceMu replaces initWG? i'm guessing the ordering it looked like it was enforcing was already assured, just needed mutual exclusion?
| if runCancel != nil { | ||
| runCancel() | ||
| runCancel = nil | ||
| currInstanceMu.Lock() |
There was a problem hiding this comment.
why did the previous code do a trylock and just bail if it didn't work..? would it have left the plugin unpaused?
| p.mu.Lock() | ||
| defer p.mu.Unlock() | ||
|
|
||
| if p.state != instanceStateInitialized { |
There was a problem hiding this comment.
nice that paused can just be initialized
| if p.state == instanceStatePreExit { | ||
| return nil | ||
| } | ||
| if p.state != instanceStateRunnable { |
There was a problem hiding this comment.
i notice this can newly cause FLBPluginOutputPreExit to panic
how concerned are you that there are paths through the state machine that don't make sense / arent really supposed to happen but do
| p.runCancel() | ||
|
|
||
| // Wait for any running callback/flush to finish before closing the message channel | ||
| p.runningWG.Wait() |
There was a problem hiding this comment.
noticed this used to not block to. i imagine this is on purpose, but any concern about unintended fallout?
|
obviously it is still in draft, but approved in concept and that once tests are passing think it is great |
This refactors the Go plugin glue code with a few goals in mind:
theInputLockor test-only branching aroundmultiInstance.This cleanly separates the stateless CGO boundary functions in cshared.go, such as
FLBPluginInit, from the stateful implementations. The C interface functions and their behavior are unchanged.The changes are largely:
pluginMetadatastruct.pluginInstancestruct.The new
TestInputCallbackLifecyclegives a sense of how this comes together. This test notably relies on no global state.You can view this PR as a precursor to supporting multiple instances of the same plugin in fluent-bit.