Skip to content

fix: fixed timeout for download promise - #5227

Closed
MarvKler wants to merge 1 commit into
MarketSquare:mainfrom
MarvKler:main
Closed

MarvKler wants to merge 1 commit into
MarketSquare:mainfrom
MarvKler:main

Conversation

@MarvKler

@MarvKler MarvKler commented Sep 7, 2026

Copy link
Copy Markdown

No description provided.

@aaltat

aaltat commented Sep 19, 2026

Copy link
Copy Markdown
Member

I looked the code and it does fix the problem, but it also leaves few other corner cases open and this PR creates a regression when timeout is not defined.

┌─────┬─────────────────────────────────────────┬─────────────────────────┬────────────────────────┬─────────────────────┐
│  #  │                Scenario                 │          main           │           PR           │      Proposal       │
├─────┼─────────────────────────────────────────┼─────────────────────────┼────────────────────────┼─────────────────────┤
│     │ Promise, server waits 3s before         │ ❌ waitForEvent:        │                        │                     │
│ A   │ responding, download_timeout=10s        │ Timeout 1000ms (the     │ ✅                     │ ✅                  │
│     │                                         │ issue)                  │                        │                     │
├─────┼─────────────────────────────────────────┼─────────────────────────┼────────────────────────┼─────────────────────┤
│ B   │ Promise, headers at once, body streams  │ ✅                      │ ✅                     │ ✅                  │
│     │ for 3s, download_timeout=10s            │                         │                        │                     │
├─────┼─────────────────────────────────────────┼─────────────────────────┼────────────────────────┼─────────────────────┤
│ C   │ Download keyword, 3s body,              │ 💥 Node process crashes │ ✅                     │ ✅                  │
│     │ download_timeout=10s                    │                         │                        │                     │
├─────┼─────────────────────────────────────────┼─────────────────────────┼────────────────────────┼─────────────────────┤
│ D   │ Promise, 3s body,                       │ ✅                      │ ✅                     │ ✅                  │
│     │ download_timeout=1.5s, should fail      │                         │                        │                     │
├─────┼─────────────────────────────────────────┼─────────────────────────┼────────────────────────┼─────────────────────┤
│     │ Promise with no download_timeout, no    │                         │ ❌ hangs forever       │                     │
│ E   │ download ever happens, should fail      │ ✅ fails after 1s       │ (stopped by a 20s test │ ✅ fails after 1s   │
│     │                                         │                         │  timeout)              │                     │
├─────┼─────────────────────────────────────────┼─────────────────────────┼────────────────────────┼─────────────────────┤
│ F   │ Download, 3s body, download_timeout=1s, │ 💥 crash                │ 💥 still crashes       │ ✅                  │
│     │  should fail cleanly                    │                         │                        │                     │
├─────┼─────────────────────────────────────────┼─────────────────────────┼────────────────────────┼─────────────────────┤
│     │                                         │                         │                        │ ✅ fails cleanly at │
│ H   │ Download, 3s body, no download_timeout  │ 💥 crash                │ ✅                     │  the browser        │
│     │                                         │                         │                        │ timeout             │
└─────┴─────────────────────────────────────────┴─────────────────────────┴────────────────────────┴─────────────────────┘

Therefore I made a another fix in #5252 therefore I am going to close this PR.

@aaltat aaltat closed this Sep 19, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants