docs: clarify PLAYWRIGHT_DOWNLOAD_CONNECTION_TIMEOUT is an idle timeout - #42601
Open
Alexandr Kazmin (alkaz-nodemaven) wants to merge 1 commit into
Open
Conversation
The variable is applied as request.setTimeout on the download request, so it bounds how long the transfer may sit idle rather than how long a connection may take to establish, and it falls back to NET_DEFAULT_TIMEOUT = 30_000 when unset. microsoft#35583 renamed the internals from *ConnectionTimeout to *SocketTimeout to avoid that confusion in the codebase; this brings the documentation along. References microsoft#28619, microsoft#42597.
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
1 participant
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.
docs/src/browsers.mdcalls this variable a connection timeout. The codebase stopped calling it that in #35583, "chore: rename *ConnectionTimeout to *SocketTimeout", whose body is "This is to avoid confusion in our codebase." That rename touched three files underregistry/and left the docs sentence behind, so the confusion it removed from the codebase is still in the documentation.packages/playwright-core/src/server/registry/index.ts:It reaches the transfer as
request.setTimeout(...)inpackages/utils/network.tsand falls back toNET_DEFAULT_TIMEOUT = 30_000when unset.tests/installation/playwright-cdn.spec.tsexercises it in exactly that shape:playwright cdn should race with a timeoutpoints the download at anhttp.createServer(() => {})that accepts and never answers, andnpx playwright install should not hang when CDN TCP connection stallsat a server that writes the headers plus one byte and then stops. Both are established connections going idle, and neither is a connect that fails.The review on #28698 suggested "the connection and idle timeout". This says idle only, because the connect phase is not covered:
request.setTimeoutis handed to the socket once it is connected, so while an agent has not yet produced a connected socket there is nothing for the timer to be armed on.why this sentence is worth a PR
It was reported as #28619 and closed with "we consider this a documentation problem. If you would like to issue a PR which clarifies the environment variable, feel free to do so." #28698 followed, picked up a suggested wording in review, and was closed; the sentence was never changed and reads the same on
maintoday.It is still costing triage. #42597, opened this week, sets the variable to 120000, watches every download attempt fail at 21 s, and reports it as a 1.63.0 regression. The failure there is an unroutable proxy address, but the reason the variable looked like the fix is this sentence.
The change keeps the variable name and every example untouched and only says what the variable does, with the 30 s default named so the number is not a mystery.
Per CONTRIBUTING this is a minor documentation fix, which is the stated exception to the linked-issue requirement, and #28619 carries an explicit invitation to send it.