Skip to content

chore: use Promise.withResolvers in SOCKS5 proxy agent#4978

Merged
mcollina merged 3 commits intonodejs:mainfrom
trivikr:lib-promise-with-resolvers
Apr 6, 2026
Merged

chore: use Promise.withResolvers in SOCKS5 proxy agent#4978
mcollina merged 3 commits intonodejs:mainfrom
trivikr:lib-promise-with-resolvers

Conversation

@trivikr
Copy link
Copy Markdown
Member

@trivikr trivikr commented Apr 5, 2026

This relates to...

Follow-up to #4972 which replaced createDeferredPromise with Promise.withResolvers()

Rationale

This PR narrows a broader Promise.withResolvers() refactor down to the places where it is most useful in the SOCKS5 proxy flow.

The initial change applied the pattern across several async helpers, but the follow-up commits intentionally restored the simpler new Promise(...) form in lower-value cases to avoid overhead. The resulting PR focuses on the SOCKS5 proxy agent, where separating promise creation from event wiring makes the connection/authentication/TLS flow easier to follow.

Changes

  • Refactor the SOCKS5 proxy socket connection flow to use Promise.withResolvers()
  • Refactor the SOCKS5 authentication wait path to use Promise.withResolvers()
  • Refactor the SOCKS5 tunnel connection wait path to use Promise.withResolvers()
  • Refactor the TLS upgrade wait path for HTTPS-over-SOCKS connections to use Promise.withResolvers()

Features

N/A

Bug Fixes

N/A

Breaking Changes and Deprecations

N/A

Status

- Replace manual promise wrappers across API and dispatcher paths
- Keep error and timeout handling intact while simplifying control flow

Signed-off-by: Kamat, Trivikram <16024985+trivikr@users.noreply.github.com>
Copy link
Copy Markdown
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.

Most of those are not useful in making the code more readable and they add more overhead (destructuring costs) but the socks5 changes are ok

@codecov-commenter
Copy link
Copy Markdown

codecov-commenter commented Apr 5, 2026

Codecov Report

❌ Patch coverage is 82.35294% with 12 lines in your changes missing coverage. Please review.
✅ Project coverage is 92.94%. Comparing base (6a30f74) to head (25d35dc).

Files with missing lines Patch % Lines
lib/dispatcher/socks5-proxy-agent.js 82.35% 12 Missing ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##             main    #4978   +/-   ##
=======================================
  Coverage   92.94%   92.94%           
=======================================
  Files         110      110           
  Lines       35767    35773    +6     
=======================================
+ Hits        33244    33251    +7     
+ Misses       2523     2522    -1     

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

@trivikr trivikr requested a review from mcollina April 5, 2026 21:10
@trivikr trivikr changed the title chore: use Promise.withResolvers for async helpers chore: use Promise.withResolvers in SOCKS5 proxy agent Apr 5, 2026
Copy link
Copy Markdown
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 bbd66d0 into nodejs:main Apr 6, 2026
35 checks passed
@trivikr trivikr deleted the lib-promise-with-resolvers branch April 6, 2026 12:31
@github-actions github-actions bot mentioned this pull request Apr 12, 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.

3 participants