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

chore: upgrade llhttp to 9.2.0 #2705

Merged
merged 3 commits into from
Feb 9, 2024
Merged

Conversation

Uzlopak
Copy link
Contributor

@Uzlopak Uzlopak commented Feb 6, 2024

To check if this closes #2678 you can run

import { fetch } from './index-fetch.js'

const response = await fetch('https://u-vpn.unilim.fr/remote/logincheck', {
  headers: {
    accept: '*/*',
    'cache-control': 'no-store, no-cache, must-revalidate',
    'content-type': 'text/plain;charset=UTF-8',
    'if-modified-since': 'Sat, 1 Jan 2000 00:00:00 GMT',
    pragma: 'no-cache',
    Referer: 'https://u-vpn.unilim.fr/remote/login?lang=en',
    'Referrer-Policy': 'strict-origin-when-cross-origin'
  },
  // Fake credentials, still throws the error though
  body: 'ajax=1&username=a&realm=&credential=b',
  method: 'POST'
})

// Only happens here.
console.log(await response.text())
@mcollina

Can your run some benchmarks on this please?

@ShogunPanda

hpe.mjs Outdated Show resolved Hide resolved
lib/client.js Outdated Show resolved Hide resolved
@codecov-commenter
Copy link

codecov-commenter commented Feb 6, 2024

Codecov Report

Attention: 220 lines in your changes are missing coverage. Please review.

Comparison is base (e39a632) 85.54% compared to head (ff3ed56) 85.28%.
Report is 277 commits behind head on main.

Files Patch % Lines
lib/fetch/util.js 60.41% 57 Missing ⚠️
lib/fetch/index.js 66.21% 50 Missing ⚠️
lib/handler/RetryHandler.js 74.35% 30 Missing ⚠️
lib/cache/cache.js 12.12% 29 Missing ⚠️
lib/fetch/dataURL.js 79.31% 12 Missing ⚠️
lib/api/readable.js 83.92% 9 Missing ⚠️
lib/core/diagnostics.js 84.74% 9 Missing ⚠️
lib/eventsource/eventsource.js 96.09% 5 Missing ⚠️
lib/core/request.js 94.36% 4 Missing ⚠️
lib/client.js 95.83% 3 Missing ⚠️
... and 6 more
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2705      +/-   ##
==========================================
- Coverage   85.54%   85.28%   -0.27%     
==========================================
  Files          76       84       +8     
  Lines        6858     7592     +734     
==========================================
+ Hits         5867     6475     +608     
- Misses        991     1117     +126     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Contributor

@ShogunPanda ShogunPanda left a comment

Choose a reason for hiding this comment

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

LGTM!

@Uzlopak
Copy link
Contributor Author

Uzlopak commented Feb 6, 2024

fixed the tests

imho ready for merge.

@Uzlopak Uzlopak changed the title upgrade llhttp chore: upgrade llhttp to 9.1.3 + Feb 6, 2024
@Uzlopak
Copy link
Contributor Author

Uzlopak commented Feb 6, 2024

@mcollina

Maybe we should first release a undici 6.6.2 before we merge this?

@ShogunPanda
Copy link
Contributor

I'm still not sure about the right semver change but yes, let's merge this first.

@Uzlopak
Copy link
Contributor Author

Uzlopak commented Feb 6, 2024

@ShogunPanda

I took the version from HEAD of the default branch of llhttp. Can you make a release of llhttp? THan we just fix the version in this PR.

@ShogunPanda
Copy link
Contributor

Sure, I'll release llhttp after lunch.

@ShogunPanda
Copy link
Contributor

@Uzlopak llhttp 9.2.0. has just been released. You can quickly rework this PR and then we're ready to go.

@Uzlopak
Copy link
Contributor Author

Uzlopak commented Feb 7, 2024

@ShogunPanda

Thanks. I just patched the version in the header file.

Copy link
Contributor

@ShogunPanda ShogunPanda left a comment

Choose a reason for hiding this comment

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

LGTM!

@Uzlopak Uzlopak changed the title chore: upgrade llhttp to 9.1.3 + chore: upgrade llhttp to 9.2.0 Feb 7, 2024
Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

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

lgtm

I think we should add support/expose the leniency flags, and potentially read those values from node.js core itself. That can be done in a follow-up PR.

@Uzlopak Uzlopak changed the base branch from main to next February 9, 2024 11:34
@Uzlopak
Copy link
Contributor Author

Uzlopak commented Feb 9, 2024

@mcollina
@ShogunPanda
@metcoder95
@KhafraDev

Target changed to next branch

Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

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

lgtm

@mcollina mcollina merged commit c2c293f into nodejs:next Feb 9, 2024
17 checks passed
@Uzlopak Uzlopak deleted the upgrade-llhttp branch February 9, 2024 18:05
ronag pushed a commit that referenced this pull request Feb 16, 2024
* upgrade llhttp

* fix tests

* set version of llhttp 9.2.0
ronag pushed a commit that referenced this pull request Feb 16, 2024
* upgrade llhttp

* fix tests

* set version of llhttp 9.2.0
ronag added a commit that referenced this pull request Feb 16, 2024
* remove anti-pattern dispatcher hooks (#2723)

* chore: upgrade llhttp to 9.2.0 (#2705)

* upgrade llhttp

* fix tests

* set version of llhttp 9.2.0

---------

Co-authored-by: Aras Abbasi <aras.abbasi@googlemail.com>
ronag pushed a commit that referenced this pull request Feb 16, 2024
* upgrade llhttp

* fix tests

* set version of llhttp 9.2.0
ronag pushed a commit that referenced this pull request Feb 26, 2024
* upgrade llhttp

* fix tests

* set version of llhttp 9.2.0
KhafraDev pushed a commit to KhafraDev/undici that referenced this pull request Jun 24, 2024
* upgrade llhttp

* fix tests

* set version of llhttp 9.2.0
@ronag ronag mentioned this pull request Jul 3, 2024
7 tasks
metcoder95 pushed a commit that referenced this pull request Jul 3, 2024
* upgrade llhttp

* fix tests

* set version of llhttp 9.2.0
metcoder95 pushed a commit that referenced this pull request Jul 3, 2024
* upgrade llhttp

* fix tests

* set version of llhttp 9.2.0
ronag pushed a commit that referenced this pull request Jul 5, 2024
* chore: upgrade llhttp to 9.2.0 (#2705)

* upgrade llhttp

* fix tests

* set version of llhttp 9.2.0

* chore: adjustments

---------

Co-authored-by: Aras Abbasi <aras.abbasi@googlemail.com>
@github-actions github-actions bot mentioned this pull request Dec 3, 2024
@github-actions github-actions bot mentioned this pull request Dec 16, 2024
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.

HPE_INVALID_CHUNK_SIZE but works without any issue in curl, browser and other runtimes excluding Node.js
6 participants