fix(floodwait): handle re-entrant invokes instead of deadlocking - #773
Open
Davincible wants to merge 1 commit into
Open
Davincible wants to merge 1 commit into
Davincible wants to merge 1 commit into
Conversation
Run's send loop calls Invoke synchronously, so it cannot dispatch anything
else until that call returns. An invoke started by another invoke therefore
never gets sent: the send loop is blocked on the outer call, and the outer
call is blocked waiting for the nested one's result. Both wait forever.
gotd/td takes exactly that path on a datacenter redirect:
upload.getFile -> FILE_MIGRATE_X
telegram/invoke.go invokeDirect -> invokeSub(targetDC)
telegram/pool.go dc -> createPool
telegram/transfer.go -> auth.exportAuthorization
exportAuthorization goes through c.invoker, which is the middleware-wrapped
invoker, so it comes straight back into Handle while the send loop is still
inside upload.getFile. The symptom is not an error but silence: the call
never returns, and a caller writing to a file leaves a zero-byte one behind.
Every cross-datacenter download in gotd/cli hangs this way.
Mark the context in send, and when Handle sees that marker, wait inline on
the calling goroutine rather than queueing. The inline path honours the same
maxWait and maxRetries bounds, so flood waits are still respected. It
deliberately does not touch per-type scheduler state: a nested call is part
of an operation already in flight, not new traffic to pace.
Scheduled traffic is unaffected. Tests cover the deadlock, flood-wait retry
on the nested path, and maxWait enforcement there; the first fails by timeout
without this change.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The problem
Run's send loop callsInvokesynchronously:So it cannot dispatch anything else until that call returns. An invoke started
by another invoke therefore never gets sent: the send loop is blocked on the
outer call, and the outer call is blocked waiting for the nested one's result
on
r.result. Both wait forever.This is not hypothetical.
gotd/tdtakes exactly that path on a datacenterredirect:
exportAuthcallsc.tg.AuthExportAuthorization(ctx, dcID), andc.tgistg.NewClient(c.invoker)— the middleware-wrapped invoker. So it comes straightback into
Handlewhile the send loop is still insideupload.getFile.The symptom is not an error, it is silence. The call never returns, nothing is
logged, and a caller writing to a file leaves a zero-byte one behind. With debug
logging the trace simply stops:
Every cross-datacenter download in
gotd/clihangs this way, which in practiceis most media: the file's DC frequently differs from the account's home DC.
The fix
Mark the context in
send, and whenHandlesees that marker, wait inline onthe calling goroutine instead of queueing.
The inline path honours the same
maxWaitandmaxRetriesbounds, so floodwaits on a nested call are still respected rather than bypassed. It deliberately
does not update per-type scheduler state: a nested call is part of an
operation already in flight, not new traffic to pace.
Scheduled traffic is completely unaffected — same queue, same throttling, same
ordering. Only the re-entrant case changes, and today that case cannot complete
at all.
Alternatives considered
sendin its own goroutine. Fixes the deadlock and thedata structures are already mutex-protected, but it changes burst behaviour
for every user, since requests that are due would go out concurrently instead
of one at a time. Much larger blast radius for the same bug.
SimpleWaiterin the caller. Works, and is what I ran locally to confirmthe diagnosis, but it gives up global flood coordination entirely and leaves
the underlying bug in place for everyone else.
Verification
maxWaitenforcement there. The first fails by timeout (context deadline exceeded) without this change.go test ./middleware/floodwait/...passes, including-race.gofmtandgo vetclean.gotd/cliv0.11.0, stillcalling
floodwait.NewWaiter(), built against this branch. An 86,425-bytephoto on DC 5 from a home DC of 4 went from hanging indefinitely to
downloading in 4.1 seconds. No change to
gotd/cliwas needed.