Support/rdkemw 21295#106
Open
brendanobra wants to merge 2 commits into
Open
Conversation
Contributor
There was a problem hiding this comment.
Pull request overview
This PR adjusts the Firebolt C++ transport’s JSON-RPC request timeout behavior to reduce false client-side timeouts observed in production by increasing the default response deadline and adding a runtime environment override.
Changes:
- Increase default
Config::waitTime_msfrom 3000ms to 5000ms (with updated rationale in the public config header template). - Add
FIREBOLT_WAIT_TIME_MSenvironment-variable override handling inGatewayImpl::connect()(validated range, warning on invalid values). - Add unit tests modeling the false-timeout scenario and validating the env override behavior.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 15 comments.
Show a summary per file
| File | Description |
|---|---|
| include/firebolt/config.h.in | Raises the default RPC response timeout to 5000ms and documents the motivation/override behavior. |
| src/gateway.cpp | Implements FIREBOLT_WAIT_TIME_MS parsing and applies it to the runtime wait-time used by the watchdog timeout logic. |
| test/unit/gatewayTest.cpp | Adds tests reproducing the false-timeout scenario and verifying env-var override behavior and guardrails. |
| src/utils.cpp | Modifies buildGatewayUrl() query-param handling (and currently adds debug stderr prints). |
| src/transport.cpp | Adds an additional connect-time URL log line (currently logs the full, unredacted URL). |
Comment on lines
+29
to
+31
| // DEBUG: Log input | ||
| fprintf(stderr, "[buildGatewayUrl] input='%s' legacyRPCv1=%d\n", url.c_str(), legacyRPCv1); | ||
|
|
Comment on lines
+47
to
67
| // Only add RPCv2=true if it's not already present | ||
| size_t rpcv2Pos = url.find("RPCv2=true"); | ||
| fprintf(stderr, "[buildGatewayUrl] checking RPCv2: pos=%zu (npos=%zu)\n", rpcv2Pos, std::string::npos); | ||
|
|
||
| if (rpcv2Pos == std::string::npos) | ||
| { | ||
| url += "?"; | ||
| fprintf(stderr, "[buildGatewayUrl] RPCv2 not found, adding...\n"); | ||
| if (url.find('?', hostStart) == std::string::npos) | ||
| { | ||
| url += "?"; | ||
| } | ||
| else | ||
| { | ||
| url += "&"; | ||
| } | ||
| url += "RPCv2=true"; | ||
| } | ||
| else | ||
| { | ||
| url += "&"; | ||
| fprintf(stderr, "[buildGatewayUrl] RPCv2 already present, skipping\n"); | ||
| } |
Comment on lines
+70
to
+71
| // DEBUG: Log output | ||
| fprintf(stderr, "[buildGatewayUrl] output='%s'\n", url.c_str()); |
| FIREBOLT_LOG_DEBUG("Transport", "[connect] websocket logging masks include=0x%x exclude=0x%x", | ||
| static_cast<unsigned>(include), static_cast<unsigned>(exclude)); | ||
|
|
||
| FIREBOLT_LOG_NOTICE("Transport", "[connect] Full WebSocket URL: %s", url.c_str()); |
Comment on lines
+392
to
+409
| std::thread( | ||
| [this, hdl, msg]() | ||
| { | ||
| std::this_thread::sleep_for(std::chrono::milliseconds(1600)); // arrives AFTER 1500ms timeout | ||
| try | ||
| { | ||
| auto req = nlohmann::json::parse(msg->get_payload()); | ||
| nlohmann::json resp; | ||
| resp["jsonrpc"] = "2.0"; | ||
| resp["id"] = req["id"]; | ||
| resp["result"] = true; // server would have returned success | ||
| m_server.send(hdl, resp.dump(), msg->get_opcode()); | ||
| } | ||
| catch (...) | ||
| { | ||
| } | ||
| }) | ||
| .detach(); |
Comment on lines
+1698
to
+1702
| ::setenv("FIREBOLT_WAIT_TIME_MS", "not_a_number", 1); | ||
| struct Cleanup | ||
| { | ||
| ~Cleanup() { ::unsetenv("FIREBOLT_WAIT_TIME_MS"); } | ||
| } cleanup; |
Comment on lines
+1725
to
+1729
| ::setenv("FIREBOLT_WAIT_TIME_MS", "0", 1); | ||
| struct Cleanup | ||
| { | ||
| ~Cleanup() { ::unsetenv("FIREBOLT_WAIT_TIME_MS"); } | ||
| } cleanup; |
Comment on lines
+1749
to
+1753
| ::setenv("FIREBOLT_WAIT_TIME_MS", "200000", 1); | ||
| struct Cleanup | ||
| { | ||
| ~Cleanup() { ::unsetenv("FIREBOLT_WAIT_TIME_MS"); } | ||
| } cleanup; |
| // --------------------------------------------------------------------------- | ||
| TEST_F(GatewayUTest, NoEnvWaitTimeMsUsesConfigValue) | ||
| { | ||
| ::unsetenv("FIREBOLT_WAIT_TIME_MS"); // ensure absent |
Comment on lines
+553
to
+556
| if (const char* env = std::getenv("FIREBOLT_WAIT_TIME_MS")) | ||
| { | ||
| char* end = nullptr; | ||
| unsigned long override_val = std::strtoul(env, &end, 10); |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| m_messageHandler = [this](connection_hdl hdl, server::message_ptr msg) | ||
| { | ||
| std::thread( | ||
| [this, hdl, msg]() |
| m_messageHandler = [this](connection_hdl hdl, server::message_ptr msg) | ||
| { | ||
| std::thread( | ||
| [this, hdl, msg]() |
| m_messageHandler = [this](connection_hdl hdl, server::message_ptr msg) | ||
| { | ||
| std::thread( | ||
| [this, hdl, msg]() |
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.
Fix: Increase default waitTime_ms to 5000ms and add FIREBOLT_WAIT_TIME_MS env override
Measured issue from prod logs
The previous default
waitTime_ms=3000mscould cause false timeouts when a backend operation legitimately takes close to (but under) the threshold. Measured real-world latency for certain Firebolt API calls in production reached p99=2493ms, leaving only a 507ms margin before thecheckPromises()watchdog rejected the in-flight request asError::Timedout.When
checkPromises()fires before the server response arrives, the caller receives a false timeout even though the server completed the operation successfully. The caller may then disconnect, reconnect, and retry — repeating multiple times and blocking the calling thread unnecessarily.The same false-timeout cycle can cause duplicate API submissions when the caller retries an operation the server already processed.
Changes
include/firebolt/config.h.inwaitTime_msincreased from 3000ms → 5000mssrc/gateway.cppFIREBOLT_WAIT_TIME_MSenvironment variable override read atconnect()timetest/unit/gatewayTest.cpp— 7 new tests (136 total, all passing):RequestFalseTimeoutWhenServerRespondsLate— negative/bug: server responds after the timeout fires; caller receivesError::Timedoutdespite server success. Directly models the defect.EnvWaitTimeMsFixesFalseTimeout— positive/fix: same conditions withFIREBOLT_WAIT_TIME_MS=5000; response arrives in time and call succeeds.EnvWaitTimeMsOverridesConfig— valid env var replaces config valueEnvWaitTimeMsInvalidValuesIgnored— non-numeric input ignoredEnvWaitTimeMsZeroIgnored— zero rejectedEnvWaitTimeMsAboveCapIgnored— value > 120000 rejectedNoEnvWaitTimeMsUsesConfigValue— absent env var leaves config unchanged (regression guard)Why 5000ms
Side effects
waitTime_msis a deadline, not a delay.connect_attempt_timeout_ms(separate field, unchanged at 10000ms).Testing