feat: allow connection header in request #1829

metcoder95 · 2022-12-22T21:10:52Z

Follow up from #1747

References

codecov-commenter · 2022-12-22T21:15:31Z

Codecov Report

Base: 90.12% // Head: 90.03% // Decreases project coverage by -0.08% ⚠️

Coverage data is based on head (42914fe) compared to base (f376a12).
Patch coverage: 54.54% of modified lines in pull request are covered.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1829      +/-   ##
==========================================
- Coverage   90.12%   90.03%   -0.09%     
==========================================
  Files          66       66              
  Lines        5815     5813       -2     
==========================================
- Hits         5241     5234       -7     
- Misses        574      579       +5

Impacted Files	Coverage Δ
lib/core/request.js	`96.55% <37.50%> (-2.85%)`	⬇️
lib/client.js	`97.53% <100.00%> (+<0.01%)`	⬆️
lib/fetch/util.js	`86.49% <0.00%> (-0.03%)`	⬇️
lib/fetch/body.js	`97.15% <0.00%> (+0.21%)`	⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

lib/client.js

mcollina

lgtm

metcoder95 · 2022-12-26T20:51:11Z

Sorry for the noise, finger issue 😅
I'll add the documentation soon, in the meanwhile is ready for review. Thanks 🙂

ronag

Why isn't it enough to have reset on the request? I don't think you need a reset option on the client.

metcoder95 · 2022-12-27T17:00:37Z

Why isn't it enough to have reset on the request? I don't think you need a reset option on the client.

Is it true that the Client does not need it at all and for instance, the parent implementations neither as well, my rationing was to allow the implementation side to have granular control over it.
Meaning that if they want to set it as the default behavior or just for a couple of requests, they have the chance to do so without having to keep track of appending the reset option on every request if they want to make this the default one.

e.g.

Circuit Breaker that transitions into half-open so it starts assessing the connection by sending requests with connection: close; and transitions to closed if the connection becomes stable.

But happy to revert it if everybody agrees this might be an overhead 🙂

ronag · 2022-12-27T17:48:21Z

You can set pipelining to 0 to achieve the same effect.

anonrig · 2023-01-01T21:22:29Z

lib/client.js

@@ -115,6 +117,10 @@ class Client extends DispatcherBase {
      throw new InvalidArgumentError('unsupported keepAlive, use pipelining=0 instead')
    }

+    if (reset != null && typeof reset !== 'boolean') {
+      throw new InvalidArgumentError('invalid reset')


in order to have a consistent error message, this should be the same as the previous error message reset must be a boolean

anonrig · 2023-01-01T21:23:30Z

lib/client.js

+  if (reset) {
+    socket[kReset] = true
+  }


We can move this to line 1388

anonrig · 2023-01-01T21:23:52Z

lib/client.js

  if (blocking) {
    socket[kBlocking] = true
  }

+  const shouldReset = request.reset != null ? request.reset : client[kClientResetValue]


Nit: request.reset ?? client[kClientResetValue]

Note: this would drop support for v12 and you'd need to remove it from the ci.

Ah, that's true. I recommend doing it in a new pull request, if we are going to drop it (and I think we should)

anonrig · 2023-01-01T21:26:29Z

lib/pool.js

@@ -40,6 +41,10 @@ class Pool extends PoolBase {
      throw new InvalidArgumentError('invalid connections')
    }

+    if (reset != null && reset.constructor !== Boolean) {


Why not typeof reset !== 'boolean'?

Was mostly about a way of working, but reverted it to keep in sync with previous @KhafraDev suggestion as well 👍

metcoder95 · 2023-01-02T12:55:20Z

I addressed the changes requested and reverted the extension to Client to support the reset property. Now, this is only accessible when executing any call that promotes the DispatchOptions-like object. I also added the documentation on top. 🙂

cc: @ronag @KhafraDev @anonrig

lib/client.js

ronag · 2023-01-02T13:25:11Z

lib/core/request.js

+      bodyTimeout,
+      reset,
+      throwOnError
+    } = options


Why change this?

Do you mean the reset?

Mostly to validate the reset value and carry over the setting during the request lifecycle, as it is attached to a single request.

Another option is to pass it as a third-parameter to the write function on lib/client. What are your thoughts? 🤔

Just add reset to the previous list. I don't understand why you moved out options.

ronag · 2023-01-02T13:25:37Z

lib/core/request.js

+    if (val.toLowerCase() === 'close') {
+      request.reset = true
+    }
+    request.headers += `${key}: ${val}\r\n`


We should not set the header here...

Better to do it in Client or where exactly?

Instead, just switching the value on the reset will do what's needed, and just check for either one of the close or keep-alive options. The settings usually come in the keep-alive header if custom behavior is desired. Thoughts? 🤔

Just remove request.headers += ${key}: ${val}\r\n`` and make sure val is either keep-alive or close.

docs/api/Dispatcher.md

Co-authored-by: Robert Nagy <[email protected]>

lib/client.js

lib/pool.js

test/request.js

lib/core/request.js

mcollina

lgtm

kibertoad · 2023-01-08T00:08:29Z

docs/api/Dispatcher.md

@@ -192,6 +192,7 @@ Returns: `Boolean` - `false` if dispatcher is busy and further dispatch calls wo
 * **origin** `string | URL`
 * **path** `string`
 * **method** `string`
+* **reset** `boolean` (optional) - Default: `false` - Indicates whether the request should attempt to create a long-living connection by sending the `connection: keep-alive` header, or close it immediately after response by sending `connection: close`.


@metcoder95 This is phrased in a confusing way. Documentation implies that "true" means creating long-lived connection, while code seems to indicate the opposite.

You're right, let me fix it opening a new PR, thanks for the heads up 🙇

robhogan · 2023-11-01T14:05:19Z

Looks like this incorrectly closed #1305 - it only partly addresses it. It still doesn't appear to be possible to customise sec-fetch-mode.

metcoder95 · 2023-11-01T20:50:12Z

Please, feel free to open a new issue to reevaluate and see further actions 🙂

robhogan · 2023-11-01T22:55:41Z

Will do, but also, please don't mark issues closed in a PR unless the issue is actually fully closed - it just creates unnecessary churn and fragmentation.

KhafraDev · 2023-11-02T00:45:04Z

setting a custom sec-fetch-mode is probably a wontfix anyways.

metcoder95 marked this pull request as draft December 22, 2022 21:11

KhafraDev reviewed Dec 22, 2022

View reviewed changes

lib/client.js Outdated Show resolved Hide resolved

mcollina approved these changes Dec 23, 2022

View reviewed changes

metcoder95 force-pushed the feat/allow_autoclose branch 3 times, most recently from f481f65 to deb3835 Compare December 26, 2022 20:50

metcoder95 marked this pull request as ready for review December 26, 2022 20:51

ronag requested changes Dec 27, 2022

View reviewed changes

anonrig requested changes Jan 1, 2023

View reviewed changes

metcoder95 force-pushed the feat/allow_autoclose branch 2 times, most recently from 6dea99e to 2a3ea3c Compare January 2, 2023 12:43

ronag reviewed Jan 2, 2023

View reviewed changes

lib/client.js Outdated Show resolved Hide resolved

ronag reviewed Jan 2, 2023

View reviewed changes

ronag and others added 9 commits January 2, 2023 15:15

feat: allow connection header in request

82da79d

feat(Client): add support for reset option to disable keep alive

f06b0a5

feat(request): add support for reset option

9113050

fix: backwards issue

8e2673a

test: add client test for reset option

eea5735

fix: order of reset prescedence

fe68676

refactor: apply review feedback

c79a6c2

refactor: remove unnecessary code

412b9a8

feat(Pool): add support for reset

8632fbc

ronag reviewed Jan 2, 2023

View reviewed changes

docs/api/Dispatcher.md Outdated Show resolved Hide resolved

revert: revert support for reset on client

0651207

metcoder95 added 4 commits January 2, 2023 15:18

refactor: adjustments accordingly to review

4349c33

fix: leftover

ed6fd0e

docs: add documentation about new option

ae04e7d

refactor: Pool leftover

d516952

metcoder95 force-pushed the feat/allow_autoclose branch from d34e35a to d516952 Compare January 2, 2023 14:25

metcoder95 and others added 3 commits January 2, 2023 15:31

docs: refactor docs/api/Dispatcher.md

e732ea0

Co-authored-by: Robert Nagy <[email protected]>

refactor: set types in sync with documentation

9e78222

test: remove leftover

96d74fc

ronag reviewed Jan 3, 2023

View reviewed changes

lib/client.js Outdated Show resolved Hide resolved

ronag reviewed Jan 3, 2023

View reviewed changes

lib/pool.js Outdated Show resolved Hide resolved

KhafraDev reviewed Jan 3, 2023

View reviewed changes

test/request.js Outdated Show resolved Hide resolved

ronag reviewed Jan 3, 2023

View reviewed changes

lib/core/request.js Outdated Show resolved Hide resolved

refactor: address review requests

42914fe

ronag approved these changes Jan 4, 2023

View reviewed changes

metcoder95 added 2 commits January 4, 2023 16:56

test: add test for invalid connection header

b38df21

test: add test for manual set of connection header

8208e9c

mcollina approved these changes Jan 5, 2023

View reviewed changes

mcollina merged commit 998a593 into nodejs:main Jan 5, 2023

metcoder95 deleted the feat/allow_autoclose branch January 5, 2023 12:24

kibertoad reviewed Jan 8, 2023

View reviewed changes

metcoder95 mentioned this pull request Jan 10, 2023

docs(Dispatcher): adjust documentation for reset flag #1852

Merged

anonrig pushed a commit to anonrig/undici that referenced this pull request Apr 4, 2023

feat: allow connection header in request (nodejs#1829)

8db8ebb

robhogan mentioned this pull request Jan 17, 2024

Can't remove default headers. Can't overwrite "sec-fetch-mode", "connection" headers. #1305

Closed

phapntm mentioned this pull request Feb 24, 2024

native fetch api does not respect keepalive parameter in RequestInit #2169

Closed

crysmags pushed a commit to crysmags/undici that referenced this pull request Feb 27, 2024

feat: allow connection header in request (nodejs#1829)

34d2687

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: allow connection header in request #1829

feat: allow connection header in request #1829

metcoder95 commented Dec 22, 2022 •

edited

Loading

codecov-commenter commented Dec 22, 2022 •

edited

Loading

mcollina left a comment

metcoder95 commented Dec 26, 2022

ronag left a comment

metcoder95 commented Dec 27, 2022

ronag commented Dec 27, 2022

anonrig Jan 1, 2023

anonrig Jan 1, 2023

anonrig Jan 1, 2023

KhafraDev Jan 1, 2023

anonrig Jan 1, 2023

anonrig Jan 1, 2023

metcoder95 Jan 2, 2023

metcoder95 commented Jan 2, 2023

ronag Jan 2, 2023

metcoder95 Jan 2, 2023

ronag Jan 3, 2023

ronag Jan 2, 2023

metcoder95 Jan 2, 2023

metcoder95 Jan 3, 2023

ronag Jan 3, 2023

mcollina left a comment

kibertoad Jan 8, 2023

metcoder95 Jan 10, 2023

robhogan commented Nov 1, 2023

metcoder95 commented Nov 1, 2023

robhogan commented Nov 1, 2023 •

edited

Loading

KhafraDev commented Nov 2, 2023

feat: allow connection header in request #1829

feat: allow connection header in request #1829

Conversation

metcoder95 commented Dec 22, 2022 • edited Loading

References

codecov-commenter commented Dec 22, 2022 • edited Loading

Codecov Report

mcollina left a comment

Choose a reason for hiding this comment

metcoder95 commented Dec 26, 2022

ronag left a comment

Choose a reason for hiding this comment

metcoder95 commented Dec 27, 2022

ronag commented Dec 27, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

metcoder95 commented Jan 2, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mcollina left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

robhogan commented Nov 1, 2023

metcoder95 commented Nov 1, 2023

robhogan commented Nov 1, 2023 • edited Loading

KhafraDev commented Nov 2, 2023

metcoder95 commented Dec 22, 2022 •

edited

Loading

codecov-commenter commented Dec 22, 2022 •

edited

Loading

robhogan commented Nov 1, 2023 •

edited

Loading