http: trim off brackets from IPv6 addresses with string operations

[pckrishnadas88]

Replace regex-based hostname normalization with direct string operations, selecting the most performant of three tested approaches:

Performance Hierarchy (http/proxy-config-ipv6-trim.js):

1. stringOp (chosen implementation):
   • IPv6: 267,922,975 ops/sec (15.5x faster than regex)
   • IPv4: 354,683,255 ops/sec (9.3x faster than regex)
2. normalize: 205-337M ops/sec
3. regex: 17-38M ops/sec

Before (regex):

this.hostname = hostname.replace(/^\[|\]$/g, '');
// ~17M ops/sec (IPv6) | ~38M ops/sec (IPv4)

After (optimized):

this.hostname = this.#normalizeHostname(hostname); // Trim off the brackets from IPv6 addresses.
// Private helper method for performance optimization
#normalizeHostname(hostname) {
    return hostname.startsWith('[') && hostname.endsWith(']') ?
      hostname.slice(1, -1) :
      hostname;
}
// ~267M ops/sec (IPv6) | ~354M ops/sec (IPv4)

Implementation Details:

• Uses direct startsWith('[') && endsWith(']') checks (stringOp)
• Chosen over alternative implementations after benchmarking
• Maintains 1:1 behavior with existing regex implementation
• More efficient for proxy servers handling high connection volumes

Behavior remains identical while being significantly faster.

Verification:

1. Run benchmarks:

   ./node benchmark/http/proxy-config-ipv6-trim.js

Benefits:

• Faster proxy configuration for IPv6/IPv4 addresses
• No behavior changes - purely an optimization
• Better performance for HTTP servers handling many connections.

Note: This is unrelated to #59375 (approved, awaiting merge).

[nodejs-github-bot]

Review requested:

• @nodejs/performance

[Uzlopak]

The benchmark is actually wrong. You have to write a benchmark, which uses the effected node code. This is a microbenchmark where you compare the performance of two or three implementations. This is not useful, if you want to determine if there is any performance improvement between two node versions.

[Uzlopak]

Is it even a hotpath?

[Uzlopak]

The benchmark should look like this:

'use strict';
const common = require('../common.js');

// Benchmark configuration
const bench = common.createBenchmark(main, {
  hostname: [
    '[::]',
    '127.0.0.1',
    'localhost',
    'www.example.proxy',
  ],
  n: [1e6]
}, {
  flags: ['--expose-internals'],
});

function main({ hostname, n }) {

  const { parseProxyConfigFromEnv } = require('internal/http');

  const protocol = 'https:';
  const env = {
    https_proxy: `https://${hostname}`,
  }

  // Warmup
  for (let i = 0; i < n; i++) {
    parseProxyConfigFromEnv(env, protocol);
  }

  // // Benchmark
  bench.start();
  for (let i = 0; i < n; i++) {
    parseProxyConfigFromEnv(env, protocol);
  }
  bench.end(n);
}

[H4ad]

Create a variable outside the loop to store the result of this function and add one assertion after the loop to ensure the returned value is valid

This is necessary to avoid v8 dead code elimination

[pckrishnadas88]

@H4ad is this what you suggested?

'use strict';
const common = require('../common.js');
const assert = require('assert');

// Benchmark configuration
const bench = common.createBenchmark(main, {
  hostname: [
    '[::]',
    '127.0.0.1',
    'localhost',
    'www.example.proxy',
  ],
  n: [1e6]
}, {
  flags: ['--expose-internals'],
});

function main({ hostname, n }) {
  const { parseProxyConfigFromEnv } = require('internal/http');

  const protocol = 'https:';
  const env = {
    https_proxy: `https://${hostname}`,
  };

  // Variable to store results outside the loop
  let lastResult;

  // Warmup
  for (let i = 0; i < n; i++) {
    lastResult = parseProxyConfigFromEnv(env, protocol);
  }

  // Expected hostname after parsing (square brackets removed for IPv6)
  const expectedHostname = hostname[0] === '[' ? hostname.slice(1, -1) : hostname;

  // Assertion to ensure the function returns a valid result
  assert(
    lastResult && typeof lastResult === 'object',
    'Invalid proxy config result after warmup'
  );
  assert(
    'hostname' in lastResult,
    'Proxy config result should have hostname property'
  );

  // Benchmark
  bench.start();
  for (let i = 0; i < n; i++) {
    lastResult = parseProxyConfigFromEnv(env, protocol);
  }
  bench.end(n);

  // Final validation
  assert(
    lastResult && typeof lastResult === 'object',
    'Invalid proxy config result after benchmark'
  );
  assert.strictEqual(
    lastResult.hostname,
    expectedHostname,
    `Proxy config hostname should be ${expectedHostname} (got ${lastResult.hostname})`
  );
}

[H4ad]

@pckrishnadas88 That's right

[pckrishnadas88]

Benchmark updated and pushed.

[lemire]

This looks like a very decent PR. Good work.

[pckrishnadas88]

This looks like a very decent PR. Good work.

Thank you for the feedback. Just updated the benchmark as well as suggested by @H4ad

[Uzlopak]

The warm up phase is still excessive. Instead of doing n-iterations it is enough to do 1000 iterations. For the warm up..

[pckrishnadas88]

The warm up phase is still excessive. Instead of doing n-iterations it is enough to do 1000 iterations. For the warm up..

PR updated with that change. Thank you for the valuable suggestions.

[joyeecheung]

Is it even a hotpath?

It's a path only used by http/https clients that are using proxies, and likely only ever called once throughout the lifetime of a Node.js http/https clients as part of the agent initialisation (typically, once per process to initialize the global agent), so...not really. (This path is not used by the server, FWIW).

I would suggest dropping the benchmark, that is a bit of an overkill for a code path like this. We don't do that for every single string operation we do in the initialisations that are usually only run once per application lifecycle.

[Uzlopak]

@joyeecheung
@H4ad
May i interest you in #59426

[prabhu]

This is a breaking change, since the original removes all leading and trailing brackets?

original

'[[[::1]]]' => '::1'

new

// double brackets at the end
'[::1]]' => '[::1]'

[Uzlopak]

Nope.

aras@aras-HP-ZBook-15-G3:~/workspace/eventsource$ node
Welcome to Node.js v22.18.0.
Type ".help" for more information.
> new URL('http://[[::1]]')
Uncaught TypeError: Invalid URL
    at new URL (node:internal/url:825:25) {
  code: 'ERR_INVALID_URL',
  input: 'http://[[::1]]'
}
> 

[pckrishnadas88]

Is it even a hotpath?

It's a path only used by http/https clients that are using proxies, and likely only ever called once throughout the lifetime of a Node.js http/https clients as part of the agent initialisation (typically, once per process to initialize the global agent), so...not really. (This path is not used by the server, FWIW).

I would suggest dropping the benchmark, that is a bit of an overkill for a code path like this. We don't do that for every single string operation we do in the initialisations that are usually only run once per application lifecycle.

I will remove the benchmark and update the PR soon.

[pckrishnadas88]

Is it even a hotpath?

It's a path only used by http/https clients that are using proxies, and likely only ever called once throughout the lifetime of a Node.js http/https clients as part of the agent initialisation (typically, once per process to initialize the global agent), so...not really. (This path is not used by the server, FWIW).

I would suggest dropping the benchmark, that is a bit of an overkill for a code path like this. We don't do that for every single string operation we do in the initialisations that are usually only run once per application lifecycle.

Benchmark file removed as suggested.

[codecov]

Codecov Report

❌ Patch coverage is 66.66667% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 89.88%. Comparing base (dfee0b1) to head (e0b8723).
⚠️ Report is 238 commits behind head on main.

Files with missing lines	Patch %	Lines
lib/internal/http.js	66.66%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #59420      +/-   ##
==========================================
- Coverage   89.89%   89.88%   -0.02%     
==========================================
  Files         656      656              
  Lines      193141   193143       +2     
  Branches    37886    37884       -2     
==========================================
- Hits       173623   173599      -24     
- Misses      12051    12056       +5     
- Partials     7467     7488      +21     

Files with missing lines	Coverage Δ
lib/internal/http.js	95.31% <66.66%> (-0.36%)	⬇️

... and 31 files with indirect coverage changes

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

[Uzlopak]

I know my vote is irrelevant, but atleast it removes my blocker ;)

[pckrishnadas88]

I know my vote is irrelevant, but atleast it removes my blocker ;)

Thank you. Learned a lot from your review.

[pckrishnadas88]

@lemire The test-macOS (pull_request) job has failed several times (around 35 min mark). The CI failure is from test-debugger-run-after-quit-restart.js on macOS. It looks unrelated to my changes — could this be a flaky test?

[tniessen]

Thanks @pckrishnadas88. I am +0 on this sort of change.

FWIW, I generally do not recommend alleging concrete measurements of performance gains of such micro-optimizations in commit messages, for a number of reasons. Benchmarking such tiny changes, especially when not on a hot path, across all supported platforms and CPU architectures is tricky, and even if done properly, it may be difficult to claim any particular one numeric factor as the ultimate performance improvement.

[pckrishnadas88]

Thanks @pckrishnadas88. I am +0 on this sort of change.

FWIW, I generally do not recommend alleging concrete measurements of performance gains of such micro-optimizations in commit messages, for a number of reasons. Benchmarking such tiny changes, especially when not on a hot path, across all supported platforms and CPU architectures is tricky, and even if done properly, it may be difficult to claim any particular one numeric factor as the ultimate performance improvement.

The benchmark file is removed already as per previous review. I recently started on contributing to Node.js this is my second PR so learning things now. Mistakes are not intentional and happy to correct as per the reviews. Thank you for the detailed explanations.

[joyeecheung]

I agree with @tniessen - it's better to drop the performance statement in the commit message, because in the case of the code being changed here, that's very likely to be untrue. This code is likely only ever executed once per process, and the optimizing compiler is not likely to touch it. Performance numbers from repeatedly calling it in a loop where the optimizing compiler would kick in are not applicable to the actual use case of this code - it's mostly likely only interpreted.

[joyeecheung]

Can you squash the commit and amend the commit message? I think something like this would work:

http: trim off brackets from IPv6 addresses with string operations

This is simpler than using regular expressions.

[pckrishnadas88]

Can you squash the commit and amend the commit message? I think something like this would work:

http: trim off brackets from IPv6 addresses with string operations

This is simpler than using regular expressions.

This change has been done. Please let me know if anything else is required.

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/68729/

[mcollina]

lgtm

[nodejs-github-bot]

Landed in 6c215fb

[pckrishnadas88]

Thanks everyone for the reviews and guidance! I appreciate the time and feedback from all of you in helping this land. 🙏

[richardlau]

This doesn't land cleanly on v22.x-staging so a manual backport will be necessary if this is to be released there. It probably needs to be backported together with the proxy stuff.