zedcloud/deferred: don't take lock while processing the deferred queue #3159
Conversation
The ctx.lock protects the concurrent access to the ctx.deferredItems, so no need to keep the mutex locked while processing the queue. Why? Queue processing can take up to several minutes (depends on the send timeout), so that all other goroutines which produce new requests (callers of the SetDeferred() or RemoveDeferred() calls) will stuck for as long as the queue is not completely processed. This mutex contention badly impacts overall system responsiveness. Signed-off-by: Roman Penyaev <r.peniaev@gmail.com>
There was a problem hiding this comment.
LGTM, but based on the issue you were debugging there is still an issue that the main event look in zedagent can get stuck inside HandleDeferred for 6 minutes during which time it does not process any other events such as subscriptions, hence e.g., AppInstanceStatus will not be reflected in what is sent to LPS during that time.
So in addition to this PR I think we need to remove the calls to HandleDeferred from the event loop and have a timer+trigger channel goroutine do all the work.
| @@ -93,23 +93,24 @@ func CreateDeferredCtx(zedcloudCtx *ZedCloudContext, sentHandler *SentHandlerFun | |||
| func (ctx *DeferredContext) HandleDeferred(event time.Time, | |||
There was a problem hiding this comment.
I think it would make sense to update the comment where it says "try to send all" since it only tries to send the deferred items which were present when the function started and needs to be called again to send any items which were added while it was running.
This already done by @milan-zededa commits, which we tested and backported to the 8.12, but not yet published. Not |
Ack. |
The
ctx.lockprotects the concurrent access to thectx.deferredItems, so no need to keep the mutex locked while processing the queue.Why? Queue processing can take up to several minutes (depends on the send timeout), so that all other goroutines which produce new requests (callers of the SetDeferred() or RemoveDeferred() calls) will stuck for as long as the queue is not completely processed. This mutex contention badly impacts overall system responsiveness.
CC: @milan-zededa