Skip to content

Comments

Do not propagate exception inside HTMLRewriter parser#5976

Merged
ryanking13 merged 1 commit intomainfrom
gyeongjae/html-rewriter-posion-after-exception
Feb 4, 2026
Merged

Do not propagate exception inside HTMLRewriter parser#5976
ryanking13 merged 1 commit intomainfrom
gyeongjae/html-rewriter-posion-after-exception

Conversation

@ryanking13
Copy link
Contributor

When an exception is threw inside HTMLRewriter handler, that error marks the task as failure and make subsequent transforms fail. This PR changes it to catch the exception so it is not propagated other than the stream.

@ryanking13 ryanking13 requested review from a team as code owners January 28, 2026 08:49
@codecov-commenter
Copy link

codecov-commenter commented Jan 28, 2026

Codecov Report

❌ Patch coverage is 80.00000% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 70.17%. Comparing base (012da41) to head (8faa650).

Files with missing lines Patch % Lines
src/workerd/api/html-rewriter.c++ 80.00% 0 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #5976      +/-   ##
==========================================
- Coverage   70.17%   70.17%   -0.01%     
==========================================
  Files         407      407              
  Lines      107238   107241       +3     
  Branches    17985    17985              
==========================================
  Hits        75257    75257              
- Misses      21195    21196       +1     
- Partials    10786    10788       +2     

☔ 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.

@ryanking13 ryanking13 force-pushed the gyeongjae/html-rewriter-posion-after-exception branch 2 times, most recently from b5b3a4a to bd2e85b Compare January 29, 2026 05:54
@codspeed-hq
Copy link

codspeed-hq bot commented Jan 29, 2026

CodSpeed Performance Report

Merging this PR will not alter performance

Comparing gyeongjae/html-rewriter-posion-after-exception (8faa650) with main (012da41)

Summary

✅ 70 untouched benchmarks
⏩ 129 skipped benchmarks1

Footnotes

  1. 129 benchmarks were skipped, so the baseline results were used instead. If they were deleted from the codebase, click here and archive them to remove them from the performance reports.

Copy link
Contributor

@dom96 dom96 left a comment

Choose a reason for hiding this comment

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

Could this have any impact on existing code in prod? i.e. do we need to do this behind a new compat flag?

@jasnell
Copy link
Collaborator

jasnell commented Feb 2, 2026

@dom96 it should not have a negative impact. This is undoing part of another recent change that landed within the past two weeks.

@ryanking13 ryanking13 force-pushed the gyeongjae/html-rewriter-posion-after-exception branch from bd2e85b to ccb8ecd Compare February 3, 2026 06:01
@ryanking13 ryanking13 enabled auto-merge February 3, 2026 06:46
@ryanking13 ryanking13 force-pushed the gyeongjae/html-rewriter-posion-after-exception branch from ccb8ecd to 8faa650 Compare February 4, 2026 05:51
@ryanking13 ryanking13 merged commit 18fcebb into main Feb 4, 2026
34 of 35 checks passed
@ryanking13 ryanking13 deleted the gyeongjae/html-rewriter-posion-after-exception branch February 4, 2026 08:13
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.

4 participants