Fix: Twitch requests time out in the first minutes of a game (BufferedHTTPClient) - #130
Fix: Twitch requests time out in the first minutes of a game (BufferedHTTPClient)#130GustavoLR548 wants to merge 1 commit into
Conversation
- Implement sequential request dispatching with a queue-based approach to ensure requests are processed one at a time. - Add traffic logging feature to diagnose stalls and timeouts. - Improvements include: tracking request dispatch timing, null-safety check in queue_free(), fixed retry logic to properly manage current_request state, and a _log_traffic() helper for timestamped diagnostics.
|
Hi Gustav, Sorry for the late reply. I didn't want to ghost you, I actually read the review of this PR a while ago but didn't know what I should do with it. On one hand the PR is technically correct but it introduces a regression problem that was solved by exactly the changes that makes the Speaking the reason why it was going in parrallel instead of sequential how it was originally planed, is because of Emoji and Badge loading. Loading them sequentially introduces always a lag when you try to load the emotes of a broadcaster. That got almost fully resolved by loading them in parallel. Reintroducing sequentiallity in this case would also break the feature on another front. I was thinking about making 2 different HTTP Clients (one sequentially buffering one for parrallel requests) but thats alot of work. Maybe as a flag in the request to signal parallel is allowed but that makes the logic more spongy. TBH I'm not sure how to handle that correctly maybe you have an Idea how to beat both flys with one stone or maybe introduce multiple stones. Best Regards |
|
Hi @kanimaru , No worries at all, thanks for explaining the context behind this! It definitely sounds like a tricky balancing act between performance and spec adherence. I can take a closer look at this and investigate the regression to see if we can find a clean way to tackle both issues. Before I dive in, how can I best test to verify that your original regression (related to the emoji/badge loading lag) and the one I found don't reappear? Are there specific benchmarks, tests, or manual steps you usually use to check this? Best, |
|
Best way to check it: That causes alot of requests depending on the streamer you pick. Also what helps with debugging it I really should create a GUT test suite for twitcher :/ But it takes soooo much time. Best Regards, |
Symptom
I am currently working a game with the plugin, and shortly after a game starts, Helix API calls fail with errors like:
result 13isHTTPRequest.RESULT_TIMEOUTandcode 0means no HTTP response was ever received — the requests never actually completed. Because they returned an empty body,JSON.parse_stringfailed and the addon misreported it as a token problem, obscuring the real cause. Thechannel.chat.messageEventSub subscription that fails as part of this batch is what stops chat from ever connecting for that session.Setting
use_threads = falseon the underlyingHTTPRequestdid not fix it, which ruled out threading as the root cause and pointed at the request queue itself.Root cause
addons/twitcher/lib/http/buffered_http_client.gdis documented as a "Http client that bufferes the requests and sends them sequentialy", but the implementation did not do that: every call torequest()immediately created and started its ownHTTPRequestnode. On startup,TwitchChat.subscribe()fires three calls back-to-back in the same frame (preload_badges,preload_emotes, and the EventSub subscription'screate_eventsub_subscriptioncall), each spinning up a parallel threadedHTTPRequest. Firing several threaded HTTP requests at once during Godot startup is exactly the pattern that stalls / times out unreliably — which is consistent with the ~30s timeout showing up on all three requests together.Two secondary bugs in the same file made the failure worse once it happened:
RESULT_CONNECTION_ERROR/RESULT_TLS_HANDSHAKE_ERROR, the retry reconnectedrequest_completedwith.bind(http_request)(the newHTTPRequestnode) instead of.bind(request_data)(the originalRequestData). Since_on_request_completedexpects aRequestData, this silently broke the retry's completion handling.retry == max_error_count, the function returned without emittingrequest_done, so anything awaitingwait_for_request()for that request would hang forever instead of receiving a (failed) response.Fix
BufferedHTTPClientnow actually queues and dispatches one request at a time per client instance (_dispatch_next()), matching its original design intent. The next request in the internal queue is only sent once the in-flight one completes (or exhausts its retries).request_datainstead of the newhttp_request, and to free the oldHTTPRequestnode instead of leaking it.max_error_countretries now clears the in-flight slot and advances the queue instead of leaving it stuck.RequestData.queue_free()now null-checkshttp_request(it can be null before the request is dispatched, now that dispatch is decoupled fromrequest()).New: optional traffic logging
Added an
@export var log_traffic: bool = falsetoggle onBufferedHTTPClient. When enabled it prints one line per request at queue time, dispatch time, and completion time, including the elapsed duration andHTTPRequest.Result/ response code — e.g.:This is off by default and meant to make future connectivity issues in this client diagnosable without re-instrumenting the code.
Files changed
addons/twitcher/lib/http/buffered_http_client.gd