Skip to content

fix(h2): TypeError: Cannot read properties of null (reading 'push') i…#4881

Merged
mcollina merged 1 commit into
nodejs:mainfrom
hxinhan:main
Mar 14, 2026
Merged

fix(h2): TypeError: Cannot read properties of null (reading 'push') i…#4881
mcollina merged 1 commit into
nodejs:mainfrom
hxinhan:main

Conversation

@hxinhan

@hxinhan hxinhan commented Mar 10, 2026

Copy link
Copy Markdown
Contributor

This relates to...

#4880

@hxinhan hxinhan force-pushed the main branch 2 times, most recently from 80a5e90 to 90a543c Compare March 10, 2026 20:32
@codecov-commenter

codecov-commenter commented Mar 10, 2026 •
edited
Loading

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 50.00000% with 4 lines in your changes missing coverage. Please review.
✅ Project coverage is 93.01%. Comparing base (07276c9) to head (eba9d9f).
⚠️ Report is 47 commits behind head on main.

Files with missing lines Patch % Lines
lib/dispatcher/client-h2.js 50.00% 4 Missing ⚠️
Additional details and impacted files 
@@            Coverage Diff             @@
##             main    #4881      +/-   ##
==========================================
- Coverage   93.02%   93.01%   -0.02%     
==========================================
  Files         112      112              
  Lines       35287    35287              
==========================================
- Hits        32827    32823       -4     
- Misses       2460     2464       +4

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

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

mcollina
mcollina requested changes Mar 11, 2026
View reviewed changes

@mcollina mcollina left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Can you solve the TOCTOU without adding the ? guard. That sit in a hot path, and it's likely that it will add some overhead.

@hxinhan

hxinhan commented Mar 11, 2026

Copy link
Copy Markdown
Contributor Author

Can you solve the TOCTOU without adding the ? guard. That sit in a hot path, and it's likely that it will add some overhead.

I'm not sure how to avoid a non-null check for res. I've removed that now.

Only with the change of moving stream.on('data', ...), the issue is still not reproducible in the regression test issue-4880.js

@hxinhan
fix(h2): TypeError: Cannot read properties of null (reading 'push') i…
eba9d9f 
…n RequestHandler.onData under high H2 concurrency
metcoder95
metcoder95 approved these changes Mar 12, 2026
View reviewed changes

@metcoder95 metcoder95 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

lgtm

mcollina
mcollina approved these changes Mar 14, 2026
View reviewed changes

@mcollina mcollina left a comment

Copy link
Copy Markdown
Member

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 9eaa5af into nodejs:main Mar 14, 2026
37 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@mcollina mcollina mcollina approved these changes

@metcoder95 metcoder95 metcoder95 approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@hxinhan @codecov-commenter @mcollina @metcoder95