fix(gsoc): client too slow handling - #5593
Conversation
gsoc.Handle spawned a goroutine per subscriber handler, so message delivery order to a subscriber was not guaranteed. Call handlers synchronously in registration order instead.
125e62e to
b15df1b
Compare
|
@nugaon |
dataC's buffer of 2 was too small for a legitimate burst of GSOC messages delivered concurrently to the same subscriber (e.g. several chunks pushed at once), causing a false-positive slow-consumer disconnect. Bump it to 16 and adjust the slow-consumer test's message count so overflow is still hit deterministically.
|
Thanks for flagging — the ordering fix wasn't the full story. The remaining flakiness in the "parallel" ci-gsoc case (10 messages delivered concurrently to one subscriber) was a false-positive "slow consumer" disconnect: the websocket delivery channel only buffered 2 messages, so a legitimate burst tripped the overflow check meant for genuinely stuck clients. Bumped the buffer to 16 in 0be4122 ( |
| go func(hh Handler) { | ||
| hh(c) | ||
| }(*hh) | ||
| (*hh)(c) |
There was a problem hiding this comment.
Could calling subscribers synchronously here delay pushsync or pullsync ?
| // Buffered enough to absorb a legitimate burst of concurrently delivered | ||
| // GSOC messages (e.g. several chunks pushed to this address at once) | ||
| // without tripping the slow-consumer detection below. | ||
| dataC = make(chan []byte, 16) |
There was a problem hiding this comment.
Not sure why this is needed in this PR. Is there any reasoning behind why 16 is the right value to be used here? If there is truly a burst > 16 we will still trip the slow-consumer detection below. Even the previous value of 2 is questionable.
If this is an actual problem, the correct fix would be something like a time-windowed rate check, or a slightly larger drain-then-reconsider window, or just accepting that a truly overwhelmed consumer should be disconnected and tuning based on expected real-world message rates than using some arbitrary constant. Having this change in this PR feels like the value is made up so the tests pass. It would be better to not have this in the current PR and making a proper fix in a subsequent PR.
There was a problem hiding this comment.
The time-windowed rate check wouldn't be the best fit since the data feed is sporadic.
I don't know what would be the optimal value, wdyt?
we can remove the whole condition as well if you are fine with that.
There was a problem hiding this comment.
anyway, I think 16 is a fine number, ofc the test scenario a bit unreal but in case of a really popular gsoc topic it can happen 16 messages arrive the same time. ofc, even more popular topic would get maybe 32...
| go func(hh Handler) { | ||
| hh(c) | ||
| }(*hh) | ||
| (*hh)(c) |
There was a problem hiding this comment.
Handle itself can still be invoked concurrently by multiple peer-connection goroutines in pushsync/pullsync for the same GSOC address (different peers forwarding the same or different chunks around the same time) — sequential dispatch only guarantees ordering within one Handle call, not across concurrent calls from different peers. If true ordering matters end-to-end, that's a separate, larger synchronization question this doesn't address.
There was a problem hiding this comment.
yeah, I see.
well, the order is not that much of importance, since GSOC does not guarantee any order anyway.
There was a problem hiding this comment.
There is no order in GSOC.
There was a problem hiding this comment.
I meant with this part that even Graffiti Feed couldn't keep canonical order, so is GSOC.
There was a problem hiding this comment.
okay, order can happen for one client if the updates' lengths are more than pull-sync time.
in a smaller network, maybe push-sync time.
|
@nugaon I pushed |
1cfebd1 to
7375a24
Compare
nugaon
changed the title
5 participants
addressing #5497 (comment)