Added timeout handling to avoid getting stuck when a website or service is not responding to a request#1133
Added timeout handling to avoid getting stuck when a website or service is not responding to a request#1133RaMaTHA wants to merge 2 commits intodevcontainers:mainfrom
Conversation
…ce is not responding to a request
|
Hello @RaMaTHA, thank you for your contribution! I agree that this is an annoying issue, but I worry that the approach in this PR will mask genuine errors or network issues, particularly because it does not throw an exception when failing. I also found that there are more idiomatic approaches now to cancelling node-http requests, such as the ones mentioned in this SO thread: https://stackoverflow.com/a/55021202 If you could implement the timeout mentioned in that thread (which applies to the connection part only), and throw an exception when timing out, I think this could be acceptable. |
|
Hello @abdurriq, thank you for your fast response. I have had a look at your suggestion regarding the timeout handling as mentioned in the SO thread you provided, and I agree that implementing a timeout for the connection part only is a better way of doing it. I have adjusted the code accordingly. |
chrmarti
left a comment
There was a problem hiding this comment.
Left a comment, maybe a flag to skip downloading the manifest would be more targeted.
| import { Log, LogLevel } from './log'; | ||
| import { readLocalFile } from './pfs'; | ||
|
|
||
| export async function request(options: { type: string; url: string; headers: Record<string, string>; data?: Buffer }, output: Log) { |
There was a problem hiding this comment.
This is also used for downloading features and we don't want to fail on these for users with a slow (could be temporarily slow) connection. I'm not sure what a good value could be here.
There was a problem hiding this comment.
According to SO (see link from @abdurriq) and to the docs:
timeout: A number specifying the socket timeout in milliseconds. This will set the timeout before the socket is connected.
So a bad connection shouldn't be an issue here, as long as it is capable of reaching the server (creating a connection). After connecting to the server, the timeout is irrelevant. Of course, the client must reach the server within the time (at the moment 3000ms).
There is also a SO discussion about server timeout (see). However, there is no ideal time to choose, but they suggest holding it as low as possible (even though that nginx has a proxy_connect_timeout 60s).
Summary
This PR adds timeout handling for HTTP/HTTPS requests to prevent builds from hanging indefinitely when a remote service is unreachable or unresponsive.
Problem
While testing in an isolated environment, I noticed that HTTP requests made by the CLI do not have a timeout configured. If the target service cannot be reached, the request may wait indefinitely, causing the build to hang.
This became visible when running the CLI on a runner that is not capable of performing outbound HTTP requests. In this scenario, the build gets stuck waiting for a response that will never arrive.
Affected endpoint
One example where this occurs is the request to:
handled in:
src/spec-configuration/controlManifest.tsSolution
A timeout has been added to the HTTP request to ensure the process fails gracefully instead of hanging indefinitely when the service is unavailable.