feat(client): add timeout to fetchResource to prevent hangs
#2322
Conversation
This file contains 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
fortuna
approved these changes
Jan 10, 2025
|
|
||
| "github.com/Jigsaw-Code/outline-apps/client/go/outline/platerrors" | ||
| ) | ||
|
|
||
| const fetchTimeout = 10 * time.Second |
There was a problem hiding this comment.
Ideally the TS code would specify this, but this is a good mitigation.
There was a problem hiding this comment.
Ideally, TS code should combine fetchResource and the next establishVPN into a single "transaction", so both operations can be cancelled by a single context object (for example, when a user wants to connect to another server, we should cancel all existing fetchResource and establishVPN operations).
sbruens
approved these changes
Jan 11, 2025
| @@ -100,3 +101,29 @@ func TestFetchResource_BodyReadError(t *testing.T) { | |||
| require.Equal(t, platerrors.FetchConfigFailed, perr.Code) | |||
| require.Error(t, perr.Cause) | |||
| } | |||
|
|
|||
| func TestFetchResource_Timeout(t *testing.T) { | |||
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.
This PR adds a timeout to the
fetchResourcefunction. Previously,fetchResourcecould hang indefinitely if the requested resource was unavailable or extremely slow to respond. (You can use this key to test: https://httpstat.us/204?sleep=120000#Timeout)Problems without the timeout
UV_THREADPOOL_SIZE). This restricts the number of concurrent native API (N-API) calls, (for example,fetchResource). If more than fourfetchResourcecalls were blocked indefinitely, it would exhaust the thread pool, preventing other N-API calls from executing. Notably, this blocked thecloseVPNcall triggered by the "Quit" action, making users unable to quit the application.http.Getcall withinfetchResourcewould consume system resources unnecessarily, potentially leading to performance degradation or even instability in extreme cases.Note
This solution is more like a mitigation, not a complete fix, for the NodeJS thread pool limitation.
Further improvements might involve implementing a more sophisticated queuing mechanism or increasing the thread pool size (if feasible).