fix: make sure we instantiate non-main modules #93
Conversation
| @@ -251,8 +254,6 @@ func (p *CompiledPlugin) Instance(ctx context.Context, config PluginInstanceConf | |||
| if err != nil { | |||
| return nil, fmt.Errorf("failed to initialize Observe Adapter: %v", err) | |||
| } | |||
|
|
|||
| trace.Finish() | |||
There was a problem hiding this comment.
calling Finish on the trace context right after creating it didn't make sense (we're storing the trace context in the plugin struct), so i removed the call
There was a problem hiding this comment.
There is also a line:
moduleConfig = moduleConfig.WithName(strconv.Itoa(int(p.instanceCount.Add(1))))It seems to be redundant now + p.instanceCount also can be removed
There was a problem hiding this comment.
hmmm, i am not sure, the comment for p.instanceCount suggests otherwise. @evacchi what do you think? I moved the naming closer to the main module initialization to make it clearer that this naming is for the main module
There was a problem hiding this comment.
Hmm, you are correct, InstantiateModule comment states that module name can't be reused, this probably also means that other modules can't be instantiated multiple times?
There was a problem hiding this comment.
IIRC not with the same name, within the same runtime
|
@mymmrac thank you very much for the review! |
| data.Name = "main" | ||
| } | ||
|
|
||
| _, okm := modules[data.Name] | ||
| _, okm := p.modules[data.Name] | ||
|
|
||
| if data.Name == "extism:host/env" || okm { |
There was a problem hiding this comment.
There can be a case, when there are 2 modules with empty name, so both will become main which is an error
| if data.Name == "extism:host/env" || okm { | |
| if data.Name == "extism:host/env" || okm || (foundMain && data.Name == "main") { |
Also, I think there should be a check that each module has non-empty name
|
@mymmrac i found a way to make module linking work with multiple modules by wrapping the non-main modules in a host module. Can you please test this and see if it works with your use case too? |
|
@evacchi good point, i will try to run a benchmark on sunday. I was also thinking maybe we implement both? and do one or the other depending if it's necessary. For example, if the user only needs one instance or if they only have one module, we don't need to do any of this (which is probably 90+% of the cases). So if there is a perf hit, we certainly can consider that too |
|
yeah that makes sense, we could just make it a flag |
Fixes #92