Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: change headersTimeout to 300s #1924

Closed
wants to merge 7 commits into from
Closed

Conversation

kyrylkov
Copy link
Contributor

No description provided.

@codecov-commenter
Copy link

codecov-commenter commented Feb 11, 2023

Codecov Report

Base: 90.41% // Head: 90.41% // No change to project coverage 👍

Coverage data is based on head (5b359ad) compared to base (e155c6d).
Patch has no changes to coverable lines.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #1924   +/-   ##
=======================================
  Coverage   90.41%   90.41%           
=======================================
  Files          71       71           
  Lines        6123     6123           
=======================================
  Hits         5536     5536           
  Misses        587      587           

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

Copy link
Member

@ronag ronag left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Isn't it enough to change the headers timeout?

@kyrylkov
Copy link
Contributor Author

I guess. Should I modify?

@kyrylkov
Copy link
Contributor Author

Done

@mcollina
Copy link
Member

why is this just a docs change?

@kyrylkov
Copy link
Contributor Author

Sorry :( Fixed now.

@mcollina
Copy link
Member

CI is failing

@kyrylkov
Copy link
Contributor Author

It says "failed 1 of 86 tests", but what exactly I cannot find. Can you?

@KhafraDev
Copy link
Member

test/request-timeout.js

@kyrylkov
Copy link
Contributor Author

kyrylkov commented Feb 11, 2023

I am sorry but I don't see where it's failing (cannot find not ok?) and I don't see why test/request-timeout.js would fail.

Let me know if you want to keep this PR open or I should close it.

@kyrylkov
Copy link
Contributor Author

kyrylkov commented Feb 14, 2023

@KhafraDev I don't see request-timeout.js failing

image

I do see fetch/abort2.js failing and I cannot figure out why

image

image

@KhafraDev
Copy link
Member

    # Subtest: stream timeout
        1..1
        not ok 1 - Should not be called
          ---
          at:
            line: 626
            column: 9
            file: test/request-timeout.js
          stack: |
            test/request-timeout.js:626:9
            StreamHandler.onHeaders (lib/api/api-stream.js:1:20335)
            Request.onHeaders (lib/core/request.js:13:10337)
            Parser.onHeadersComplete (lib/client.js:28:96)
            wasm_on_headers_complete (lib/client.js:17:1842)
            wasm-function[11]:0x494
            wasm-function[51]:0x1003
            wasm-function[68]:0x7046
            wasm-function[67]:0x155d
            wasm-function[21]:0x552
            Parser.execute (lib/client.js:25:184)
            Parser.readMore (lib/client.js:21:391)
            Socket.onSocketReadable (lib/client.js:37:1132)
          source: "    }, (result) => {\r
          
            \      t.fail('Should not be called')\r
          
            --------^
          
            \    }, (err) => {\r
          
            \      t.type(err, errors.HeadersTimeoutError)\n"
          ...
        
        # failed 1 test
    not ok 18 - stream timeout # time=-603555.004ms
    
    # Subtest: stream custom timeout
        1..1
        ok 1 - type is HeadersTimeoutError
    ok 19 - stream custom timeout # time=-603577.07ms
    
    # Subtest: pipeline timeout
        1..1
        not ok 1 - Should not be called
          ---
          at:
            line: 707
            column: 11
            file: test/request-timeout.js
          stack: |
            test/request-timeout.js:707:11
            PipelineHandler.onHeaders (lib/api/api-pipeline.js:2:1446)
            Request.onHeaders (lib/core/request.js:13:10337)
            Parser.onHeadersComplete (lib/client.js:28:96)
            wasm_on_headers_complete (lib/client.js:17:1842)
            wasm-function[11]:0x494
            wasm-function[51]:0x1003
            wasm-function[68]:0x7046
            wasm-function[67]:0x155d
            wasm-function[21]:0x552
            Parser.execute (lib/client.js:25:184)
            Parser.readMore (lib/client.js:21:391)
            Socket.onSocketReadable (lib/client.js:37:1132)
          source: "      }, (result) => {\r
          
            \        t.fail('Should not be called')\r
          
            ----------^
          
            \      }, (e) => {\r
          
            \        t.fail('Should not be called')\n"
          ...
        
        # failed 1 test
    not ok 20 - pipeline timeout # time=-603581.501ms
    
    # Subtest: pipeline timeout
        1..1
        ok 1 - type is HeadersTimeoutError
    ok 21 - pipeline timeout # time=-603603.37ms
    
    # Subtest: client.close should not deadlock
        1..2
        ok 1 - type is HeadersTimeoutError
        ok 2 - should not error
    ok 22 - client.close should not deadlock # time=-634512.15ms
    
    not ok 23 - The client is destroyed
      ---
      at:
        line: 2
        column: 1519
        file: lib/dispatcher-base.js
        function: Client.destroy
      stack: |
        Client.destroy (lib/dispatcher-base.js:2:1519)
        lib/dispatcher-base.js:2:611
        new Promise (<anonymous>)
        Client.destroy (lib/dispatcher-base.js:2:529)
      type: ClientDestroyedError
      code: UND_ERR_DESTROYED
      tapCaught: uncaughtException
      test: TAP
      source: "'use strict'\r
      
        \r
      
        const Dispatcher = require('./dispatcher')\r
      
        const {\n"
      ...
    
    1..23
    # failed 3 of 23 tests
    # skip: 1
    # time=1238.143ms
not ok 69 - test/request-timeout.js # time=1238.143ms
  ---
  env: {}
  file: test/request-timeout.js
  timeout: 60000
  command: C:\hostedtoolcache\windows\node\12.22.12\x64\node.exe
  args:
    - --expose-gc
    - --experimental-wasm-simd
    - test/request-timeout.js
  stdio:
    - 0
    - pipe
    - 2
  cwd: D:\a\undici\undici
  exitCode: 1
  ...

@kyrylkov
Copy link
Contributor Author

@KhafraDev Pushed updated request-timeout.js, but fetch/abort2.js still failes

@KhafraDev
Copy link
Member

that test fails locally, works in the ci

@kyrylkov
Copy link
Contributor Author

kyrylkov commented Feb 14, 2023

@KhafraDev Well, that's confusing, but OK. How can CI tests be rerun after my latest commit?

@kyrylkov kyrylkov changed the title feat: change bodyTimeout and headersTimeout to 300s feat: change headersTimeout to 300s Feb 14, 2023
@KhafraDev
Copy link
Member

they should re-run automatically, I'm not sure why they weren't queued

@kyrylkov
Copy link
Contributor Author

@KhafraDev can you manually start CI?

@mcollina
Copy link
Member

you can force push or add a tiny commit to trigger them again

@kyrylkov
Copy link
Contributor Author

kyrylkov commented Feb 15, 2023

@mcollina I just force pushed, but CI didn't run again ee9c9cd

@mcollina
Copy link
Member

I don’t know what’s the problem, it might be a glitch in GitHub actions. Try opening a fresh PR.

@kyrylkov kyrylkov closed this Feb 17, 2023
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.

5 participants