improvements to context severing #2370
Conversation
The code stems from a misunderstanding with how context cancelation works: child cancelation is not propagated upwards to the parent. This caused downstream queries caused by LR2 not to be canceled when requested.
This helps making sure that all the values stuffed in the context are retained without having to recreate all of it manually
this means we keep all the different components injected in the context intact, and we only change the cancelation propagation.
| @@ -457,7 +457,7 @@ func (ls *parallelLimitedIndexedStream[Q]) forTaskIndex(ctx context.Context, ind | |||
| // Create a new cursor with cloned limits, because each child task which executes (in parallel) will need its own | |||
| // limit tracking. The overall limit on the original cursor is managed in completedTaskIndex. | |||
| childCI := currentCursor.withClonedLimits() | |||
| childContext, cancelDispatch := branchContext(ctx) | |||
| childContext, cancelDispatch := context.WithCancelCause(ctx) | |||
There was a problem hiding this comment.
The previous version severed the context completely - did you mean to write WithoutCancel here?
There was a problem hiding this comment.
I deliberately intended to not sever the context. I couldn't find a reason to justify severed context. This is what prompted this PR: I observed that cancelation wasn't getting propagated to ReverseQueryRelationship calls in the data store, which means we could be not _ freeing resources.
I checked with Joey and he also didn't recall what was the reason, other than a potential misunderstanding around child context propagating cancelation to parents (which is not the case).
But tests are failing so... 🤷🏻♂️
| @@ -66,15 +47,15 @@ func (p *ctxProxy) IsStrictReadModeEnabled() bool { | |||
| } | |||
|
|
|||
| func (p *ctxProxy) OptimizedRevision(ctx context.Context) (datastore.Revision, error) { | |||
There was a problem hiding this comment.
Is this proxy even used anywhere anymore?
There was a problem hiding this comment.
Yes, MySQL datastore still uses it.
Several improvements to the use of context-severing strategies in the codebase