fix: fix thread safety issue in engine invoke #272
Conversation
There was a problem hiding this comment.
issue(blocking): Similar to my comment #271 (comment), can we introduce a test which captures the desired behaviour before merging this item.
|
This is stalled. Are we planning on continuing this PR? |
PeterSchafer
force-pushed
the
fix/engine_concurrency
branch
from
7559cd6 to
0319c06
Compare
PeterSchafer
force-pushed
the
fix/engine_concurrency
branch
from
0319c06 to
2bd6237
Compare
PeterSchafer
dismissed
thisislawatts’s stale review
Opening up for others to review
|
@bastiandoetsch I added a test and adapted the critical section a bit. Could you please re-review? I think it would be good to get this merged. |
| @@ -247,21 +251,25 @@ func (e *EngineImpl) InvokeWithInputAndConfig( | |||
|
|
|||
| // prepare logger | |||
| prefix := fmt.Sprintf("%s:%d", id.Host, e.invocationCounter) | |||
| e.mu.Unlock() | |||
There was a problem hiding this comment.
I think, we'd in general only need a read-lock (and an RWMutex). Nothing blocking though. Should the lock maybe even start earlier, e.g. in L240 to avoid init races?
pkg/workflow/engineimpl.go
Outdated
| zlogger := e.logger.With().Str("ext", prefix).Logger() | ||
|
|
||
| localConfig := e.config | ||
| localNetwork := e.networkAccess |
There was a problem hiding this comment.
I'd find it more intuitive, to have localNetwork never contain e.networkAccess, but the clone directly on assignment - that way it is always local. Same for config.
|
|
||
| // create a context object for the invocation | ||
| context := NewInvocationContext(id, config, e, networkAccess, zlogger, e.analytics, e.ui) | ||
| context := NewInvocationContext(id, config, e, networkAccess, zlogger, localAnalytics, localUi) |
There was a problem hiding this comment.
why a new network access, if you already have the localNetwork? Confused me briefly, why networkAccess is passed instead of localNetwork
| networkAccess.SetConfiguration(config) | ||
| e.mu.Unlock() |
There was a problem hiding this comment.
do we need the additional networkAccess variable?
There was a problem hiding this comment.
Yes we need it, since we set the given configuration, which should have a side effect on the engine networkAccess.
PeterSchafer
force-pushed
the
fix/engine_concurrency
branch
from
2bd6237 to
a5b2a0a
Compare
No description provided.