fix: accept URL origin in Agent

[szmarczak]

Fixes #889

szm@solus ~/Desktop/undici $ node benchmarks/benchmark.js 
│ Tests               │ Samples │           Result │ Tolerance │ Difference with slowest │
|─────────────────────|─────────|──────────────────|───────────|─────────────────────────|
│ http - no keepalive │      60 │  5837.63 req/sec │  ± 2.88 % │                       - │
|─────────────────────|─────────|──────────────────|───────────|─────────────────────────|
│ http - keepalive    │      15 │  9187.24 req/sec │  ± 2.44 % │               + 57.38 % │
|─────────────────────|─────────|──────────────────|───────────|─────────────────────────|
│ undici - pipeline   │      25 │ 13515.93 req/sec │  ± 2.76 % │              + 131.53 % │
|─────────────────────|─────────|──────────────────|───────────|─────────────────────────|
│ undici - request    │      10 │ 18800.16 req/sec │  ± 2.41 % │              + 222.05 % │
|─────────────────────|─────────|──────────────────|───────────|─────────────────────────|
│ undici - stream     │      20 │ 20052.72 req/sec │  ± 2.55 % │              + 243.51 % │
|─────────────────────|─────────|──────────────────|───────────|─────────────────────────|
│ undici - dispatch   │      10 │ 22855.44 req/sec │  ± 2.85 % │              + 291.52 % │

[szmarczak]

I wonder if it would be better to use options.protocol, options.host and options.port instead of options.origin. That way we don't have to use new URL(...) again.

[codecov-commenter]

Codecov Report

Merging #892 (9c6ac92) into main (aebbb5d) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##             main     #892   +/-   ##
=======================================
  Coverage   99.62%   99.62%           
=======================================
  Files          26       26           
  Lines        2126     2127    +1     
=======================================
+ Hits         2118     2119    +1     
  Misses          8        8           

Impacted Files	Coverage Δ
lib/agent.js	98.05% <100.00%> (+0.01%)	⬆️
lib/core/util.js	100.00% <100.00%> (ø)
lib/client.js	99.60% <0.00%> (ø)
lib/handler/redirect.js	96.87% <0.00%> (ø)

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update aebbb5d...9c6ac92. Read the comment docs.

[szmarczak]

undici/lib/client.js

Line 161 in aebbb5d

this[kUrl] = util.parseOrigin(url)

is unsafe because:

undici/lib/core/util.js

Lines 75 to 76 in aebbb5d

	function parseOrigin (url) {
	url = parseURL(url)

undici/lib/core/util.js

Lines 58 to 72 in aebbb5d

	if (!(url instanceof URL)) {
	const port = url.port != null
	? url.port
	: { 'http:': 80, 'https:': 443 }[url.protocol]
	const origin = url.origin != null
	? url.origin
	: `${url.protocol}//${url.hostname}:${port}`
	const path = url.path != null
	? url.path
	: `${url.pathname || ''}${url.search || ''}`
	url = new URL(path, origin)
	}
	return url

meaning: it does not return new URL(...) if the we pass a URL instance. So the mutations are visible while they shouldn't be. What shall we do?

[ronag]

undici/lib/client.js

Line 161 in aebbb5d

this[kUrl] = util.parseOrigin(url)

is unsafe because:

undici/lib/core/util.js

Lines 75 to 76 in aebbb5d

	function parseOrigin (url) {
	url = parseURL(url)

undici/lib/core/util.js

Lines 58 to 72 in aebbb5d

	if (!(url instanceof URL)) {
	const port = url.port != null
	? url.port
	: { 'http:': 80, 'https:': 443 }[url.protocol]
	const origin = url.origin != null
	? url.origin
	: `${url.protocol}//${url.hostname}:${port}`
	const path = url.path != null
	? url.path
	: `${url.pathname || ''}${url.search || ''}`
	url = new URL(path, origin)
	}
	return url

meaning: it does not return new URL(...) if the we pass a URL instance. So the mutations are visible while they shouldn't be. What shall we do?

Make parseURL always copy?

[ronag]

I'm a little concerned about the performance implications. Is it negligable?

[mcollina]

Parsing URLs is slow, we should not be creating a fresh URL instance every time if possible.

[ronag]

origin is just used as a key here so we don't actually need to validate it.

this[kFactory](origin, this[kOptions]) should do the validation already.

[ronag]

let origin
if (opts.origin && (typeof opts.origin === 'string' || opts.origin instanceof URL)) {
  origin = String(opts.origin)
} else {
  throw new InvalidArgumentError('opts.origin must be a non-empty string or URL.')
}

[ronag]

Fixes #889

szm@solus ~/Desktop/undici $ node benchmarks/benchmark.js 
│ Tests               │ Samples │           Result │ Tolerance │ Difference with slowest │
|─────────────────────|─────────|──────────────────|───────────|─────────────────────────|
│ http - no keepalive │      60 │  5837.63 req/sec │  ± 2.88 % │                       - │
|─────────────────────|─────────|──────────────────|───────────|─────────────────────────|
│ http - keepalive    │      15 │  9187.24 req/sec │  ± 2.44 % │               + 57.38 % │
|─────────────────────|─────────|──────────────────|───────────|─────────────────────────|
│ undici - pipeline   │      25 │ 13515.93 req/sec │  ± 2.76 % │              + 131.53 % │
|─────────────────────|─────────|──────────────────|───────────|─────────────────────────|
│ undici - request    │      10 │ 18800.16 req/sec │  ± 2.41 % │              + 222.05 % │
|─────────────────────|─────────|──────────────────|───────────|─────────────────────────|
│ undici - stream     │      20 │ 20052.72 req/sec │  ± 2.55 % │              + 243.51 % │
|─────────────────────|─────────|──────────────────|───────────|─────────────────────────|
│ undici - dispatch   │      10 │ 22855.44 req/sec │  ± 2.85 % │              + 291.52 % │

Would be great if you did before/after. Also I don't think the benchmarks test Agent at the moment?

[ronag]

I wonder if it would be better to use options.protocol, options.host and options.port instead of options.origin. That way we don't have to use new URL(...) again.

You could let origin be an object with those properties... but then we still need to validate them.

[mcollina]

lgtm