fix: dns interceptor with Pool

[luddd3]

This relates to...

Rationale

The DNS interceptor didn't work with Pool/Client since it didn't get the origin unless included with each request.

Changes

Features

Bug Fixes

• fix dns interceptor with Client/Pool

Breaking Changes and Deprecations

Status

• I have read and agreed to the Developer's Certificate of Origin
• Tested
• Benchmarked (optional)
• Documented
• Review ready
• In review
• Merge ready

[mcollina]

lgtm

[ronag]

The origin should always be in origDispatchOpts. This seems to be a hack.

[luddd3]

The origin should always be in origDispatchOpts. This seems to be a hack.

I kinda agree, but didn't find where else to put it?

[luddd3]

I made an alternative solution. It currently breaks a test for test/interceptors/retry.js and I'm unsure if it is an actual error or not? Should I just change the expected value?

[metcoder95]

Just adjust the retry tests and should be ok to go.

The change is kind of expected as the origin is being explicitly passed and is the first origin the client is facing

[ronag]

This still looks weird... where/how does the origin get lost?

[luddd3]

The interceptor does only have access to the options called by .request(). The request method has access to all variables in this (e.g Pool) since it is bound to this here:

undici/lib/dispatcher/dispatcher.js

Line 23 in c2933ef

letdispatch=this.dispatch.bind(this)

It could probably be solved in the same manner for the interceptor by changing:

undici/lib/dispatcher/dispatcher.js

Line 34 in c2933ef

dispatch=interceptor(dispatch)

Into:

dispatch=interceptor.bind(this)(dispatch)

or something like this (which is a bit like my original approach):

dispatch=interceptor(dispatch,this)

The last method has the benefit of allowing the interceptors to return arrow functions as well as ordinary functions.

[mcollina]

THis now conflicts, can you rebase?

[ronag]

This is wrong... not sure when I have time to look into it. Christmas...

[metcoder95]

Let me try to check it during the weekend; if there's a workaround, let's take it, otherwise this should be ok for now and we can revisit later so dns is unblocked

[ronag]

The problem here is the assumption that you don't have to pass origin in the request options if used with Client or Pool, which IMHO is incorrect and it's working as intended.

If anything maybe the dns interceptor should check for origin and throw an invalid arg error. Alternatively, if no origin is passed then the dns interceptor is a noop.

[ronag]

diff --git a/lib/interceptor/dns.js b/lib/interceptor/dns.js
index c8d56c2c..c6a1a480 100644
--- a/lib/interceptor/dns.js
+++ b/lib/interceptor/dns.js
@@ -342,6 +342,10 @@ module.exports = interceptorOpts => {
return dispatch => {
return function dnsInterceptor (origDispatchOpts, handler) {
+ if (origDispatchOpts.origin == null) {
+ return dispatch(origDispatchOpts, handler)
+ }
+
const origin =

[mcollina]

lgtm

[luddd3]

The problem here is the assumption that you don't have to pass origin in the request options if used with Client or Pool, which IMHO is incorrect and it's working as intended.

@ronag That was my assumption. Just to be clear, do you think it should be like this?

constclient=newPool('https://google.com',{connections: 10})awaitclient.request({origin: 'https://google.com',// origin must be included in each requestmethod: 'GET',path: '/'})

If that is the case, then I think there are a lot of inconsistencies and contradictions in both the code and documentation.

[ronag]

If that is the case, then I think there are a lot of inconsistencies and contradictions in both the code and documentation.

Possibly. Then let's just make the dns interceptor a noop when origin is missing as I suggested with the patch.

[metcoder95]

I do agree that the documentation has some statements that are either not fully true or confusing for the reader that can lead to a wrong assumption; but that's at an overall take.

The last being said; I'm not 100% in sync with the statement that passing the origin should be mandated on every request call. I'm in sync when use through dynamic dispatchers like Agent or the BalancedPool, but not fully in sync for Client and Pool which are tight to a single origin.

For the latter use-cases, they should have the origin already stick to their state and do not ask the caller to pass it as its goal is to be tight to a single downstream.

I'm ok with the dns interceptor doing a noop if no origin is passed or if it is already an IP address; but the fact that Pool and Client does not forward the origin seems a bit of miss alignment with its purpose

[luddd3]

I think that Client and Pool actually should throw an error when origin is provided to request(), so that the caller notices that it is wrong.

Below is an example of how it works today when different origin is provided to Client during the constructor and request(). The caller might think that the request will be made to http://localhost:2000, which it won't. In this case a thrown error would be much clearer and avoid mistakes. It works similarly for Pool.

import{createServer}from'node:http'import{Client}from'undici'constserver=createServer()server.on('request',(req,res)=>{res.end('hello')})server.listen(0)constclient=newClient(`http://localhost:${server.address().port}`)constresponse=awaitclient.request({method: 'GET',origin: 'http://localhost:2000',path: '/'})console.log(awaitresponse.body.text())// will print 'hello'awaitserver.close()

[luddd3]

I'm ok with the dns interceptor doing a noop if no origin is passed or if it is already an IP address; but the fact that Pool and Client does not forward the origin seems a bit of miss alignment with its purpose

I think that it should throw an error, since it should never be the case that origin is missing. Unless it can get origin directly from Client or Pool via one of the solutions I provided earlier.

[ronag]

I think that Client and Pool actually should throw an error when origin is provided to request(), so that the caller notices that it is wrong.

It should throw when a different origin is provided. Yes.

[luddd3]

It should throw when a different origin is provided. Yes.

I can understand from the viewpoint of backwards-compatibility, but are there other benefits for not throwing every time? I'm afraid that it encourages the wrong behavior to allow an origin, which isn't used,. I have however made a commit which does that.

[mcollina]

lgtm

[ronag]

Still blocking. Please don't override request and instead ignore requests without origin in the dna interceptor

[luddd3]

Still blocking. Please don't override request and instead ignore requests without origin in the dna interceptor

@ronag

1. Why is it better to ignore requests without origin in the dns interceptor instead of throwing an error?
2. Do you mean that I should remove all code I placed in Client and Pool, or just that I shouldn't modify the request options if origin was provided?

[ronag]

The only change here should be a condition in the dns interceptor and a test.

[luddd3]

The only change here should be a condition in the dns interceptor and a test.

So basically remove all changes so far, apply the patch your provided earlier and then write a test?

[luddd3]

IMHO that doesn't solve anything. I as a user would not expect the dns interceptor to be silently bypassed when origin wasn't provided again in my request.

Everything points to origin not being necessary with Client and Pool and it is also not possible to switch origin, so why not override request and make sure that the interceptors get it? Even the examples in the documentation https://undici.nodejs.org/#/docs/api/Client?id=example-client-connect-event shows that request can be called without providing origin again.

[metcoder95]

Gentle ping

[luddd3]

Gentle ping

I don't know how to move forward since none of my proposed solutions have been accepted and I don't think the alternatives are any good. Do you have any ideas?

[metcoder95]

cc: @ronag

[ronag]

I'm not sure what's wrong with my proposal?

Expect origin to be passed to request, if it is passed, make sure it's same as the Pool/Client and if not passed then it's a noop for the dns interceptor.

[luddd3]

I'm not sure what's wrong with my proposal?

Expect origin to be passed to request, if it is passed, make sure it's same as the Pool/Client and if not passed then it's a noop for the dns interceptor.

I think it is wrong that:

1. Client and Pool forces the user to pass origin for each request. It isn't documentet and wouldn't work for different origins. fix: dns interceptor with Pool #3957 (comment)
2. the dns interceptor should be silently bypassed if a different origin is passed. I much rather it throws an error so that the user is informed that there is something wrong.

[luddd3]

Fixed with #5624

[luddd3]

This relates to...

Rationale

The DNS interceptor didn't work with Pool/Client since it didn't get the origin unless included with each request.

Changes

Features

Bug Fixes

• fix dns interceptor with Client/Pool

Breaking Changes and Deprecations

Status

• I have read and agreed to the Developer's Certificate of Origin
• Tested
• Benchmarked (optional)
• Documented
• Review ready
• In review
• Merge ready

[mcollina]

lgtm

[ronag]

The origin should always be in origDispatchOpts. This seems to be a hack.

[luddd3]

The origin should always be in origDispatchOpts. This seems to be a hack.

I kinda agree, but didn't find where else to put it?

[luddd3]

I made an alternative solution. It currently breaks a test for test/interceptors/retry.js and I'm unsure if it is an actual error or not? Should I just change the expected value?

[metcoder95]

Just adjust the retry tests and should be ok to go.

The change is kind of expected as the origin is being explicitly passed and is the first origin the client is facing

[ronag]

This still looks weird... where/how does the origin get lost?

[luddd3]

The interceptor does only have access to the options called by .request(). The request method has access to all variables in this (e.g Pool) since it is bound to this here:

undici/lib/dispatcher/dispatcher.js

Line 23 in c2933ef

letdispatch=this.dispatch.bind(this)

It could probably be solved in the same manner for the interceptor by changing:

undici/lib/dispatcher/dispatcher.js

Line 34 in c2933ef

dispatch=interceptor(dispatch)

Into:

dispatch=interceptor.bind(this)(dispatch)

or something like this (which is a bit like my original approach):

dispatch=interceptor(dispatch,this)

The last method has the benefit of allowing the interceptors to return arrow functions as well as ordinary functions.

[mcollina]

THis now conflicts, can you rebase?

[ronag]

This is wrong... not sure when I have time to look into it. Christmas...

[metcoder95]

Let me try to check it during the weekend; if there's a workaround, let's take it, otherwise this should be ok for now and we can revisit later so dns is unblocked

[ronag]

The problem here is the assumption that you don't have to pass origin in the request options if used with Client or Pool, which IMHO is incorrect and it's working as intended.

If anything maybe the dns interceptor should check for origin and throw an invalid arg error. Alternatively, if no origin is passed then the dns interceptor is a noop.

[ronag]

diff --git a/lib/interceptor/dns.js b/lib/interceptor/dns.js
index c8d56c2c..c6a1a480 100644
--- a/lib/interceptor/dns.js
+++ b/lib/interceptor/dns.js
@@ -342,6 +342,10 @@ module.exports = interceptorOpts => {
return dispatch => {
return function dnsInterceptor (origDispatchOpts, handler) {
+ if (origDispatchOpts.origin == null) {
+ return dispatch(origDispatchOpts, handler)
+ }
+
const origin =

[mcollina]

lgtm

[luddd3]

The problem here is the assumption that you don't have to pass origin in the request options if used with Client or Pool, which IMHO is incorrect and it's working as intended.

@ronag That was my assumption. Just to be clear, do you think it should be like this?

constclient=newPool('https://google.com',{connections: 10})awaitclient.request({origin: 'https://google.com',// origin must be included in each requestmethod: 'GET',path: '/'})

If that is the case, then I think there are a lot of inconsistencies and contradictions in both the code and documentation.

[ronag]

If that is the case, then I think there are a lot of inconsistencies and contradictions in both the code and documentation.

Possibly. Then let's just make the dns interceptor a noop when origin is missing as I suggested with the patch.

[metcoder95]

I do agree that the documentation has some statements that are either not fully true or confusing for the reader that can lead to a wrong assumption; but that's at an overall take.

The last being said; I'm not 100% in sync with the statement that passing the origin should be mandated on every request call. I'm in sync when use through dynamic dispatchers like Agent or the BalancedPool, but not fully in sync for Client and Pool which are tight to a single origin.

For the latter use-cases, they should have the origin already stick to their state and do not ask the caller to pass it as its goal is to be tight to a single downstream.

I'm ok with the dns interceptor doing a noop if no origin is passed or if it is already an IP address; but the fact that Pool and Client does not forward the origin seems a bit of miss alignment with its purpose

[luddd3]

I think that Client and Pool actually should throw an error when origin is provided to request(), so that the caller notices that it is wrong.

Below is an example of how it works today when different origin is provided to Client during the constructor and request(). The caller might think that the request will be made to http://localhost:2000, which it won't. In this case a thrown error would be much clearer and avoid mistakes. It works similarly for Pool.

import{createServer}from'node:http'import{Client}from'undici'constserver=createServer()server.on('request',(req,res)=>{res.end('hello')})server.listen(0)constclient=newClient(`http://localhost:${server.address().port}`)constresponse=awaitclient.request({method: 'GET',origin: 'http://localhost:2000',path: '/'})console.log(awaitresponse.body.text())// will print 'hello'awaitserver.close()

[luddd3]

I'm ok with the dns interceptor doing a noop if no origin is passed or if it is already an IP address; but the fact that Pool and Client does not forward the origin seems a bit of miss alignment with its purpose

I think that it should throw an error, since it should never be the case that origin is missing. Unless it can get origin directly from Client or Pool via one of the solutions I provided earlier.

[ronag]

I think that Client and Pool actually should throw an error when origin is provided to request(), so that the caller notices that it is wrong.

It should throw when a different origin is provided. Yes.

[luddd3]

It should throw when a different origin is provided. Yes.

I can understand from the viewpoint of backwards-compatibility, but are there other benefits for not throwing every time? I'm afraid that it encourages the wrong behavior to allow an origin, which isn't used,. I have however made a commit which does that.

[mcollina]

lgtm

[ronag]

Still blocking. Please don't override request and instead ignore requests without origin in the dna interceptor

[luddd3]

Still blocking. Please don't override request and instead ignore requests without origin in the dna interceptor

@ronag

1. Why is it better to ignore requests without origin in the dns interceptor instead of throwing an error?
2. Do you mean that I should remove all code I placed in Client and Pool, or just that I shouldn't modify the request options if origin was provided?

[ronag]

The only change here should be a condition in the dns interceptor and a test.

[luddd3]

The only change here should be a condition in the dns interceptor and a test.

So basically remove all changes so far, apply the patch your provided earlier and then write a test?

[luddd3]

IMHO that doesn't solve anything. I as a user would not expect the dns interceptor to be silently bypassed when origin wasn't provided again in my request.

Everything points to origin not being necessary with Client and Pool and it is also not possible to switch origin, so why not override request and make sure that the interceptors get it? Even the examples in the documentation https://undici.nodejs.org/#/docs/api/Client?id=example-client-connect-event shows that request can be called without providing origin again.

[metcoder95]

Gentle ping

[luddd3]

Gentle ping

I don't know how to move forward since none of my proposed solutions have been accepted and I don't think the alternatives are any good. Do you have any ideas?

[metcoder95]

cc: @ronag

[ronag]

I'm not sure what's wrong with my proposal?

Expect origin to be passed to request, if it is passed, make sure it's same as the Pool/Client and if not passed then it's a noop for the dns interceptor.

[luddd3]

I'm not sure what's wrong with my proposal?

Expect origin to be passed to request, if it is passed, make sure it's same as the Pool/Client and if not passed then it's a noop for the dns interceptor.

I think it is wrong that:

1. Client and Pool forces the user to pass origin for each request. It isn't documentet and wouldn't work for different origins. fix: dns interceptor with Pool #3957 (comment)
2. the dns interceptor should be silently bypassed if a different origin is passed. I much rather it throws an error so that the user is informed that there is something wrong.

[luddd3]

Fixed with #5624

[luddd3]

This relates to...

Rationale

The DNS interceptor didn't work with Pool/Client since it didn't get the origin unless included with each request.

Changes

Features

Bug Fixes

• fix dns interceptor with Client/Pool

Breaking Changes and Deprecations

Status

• I have read and agreed to the Developer's Certificate of Origin
• Tested
• Benchmarked (optional)
• Documented
• Review ready
• In review
• Merge ready

[mcollina]

lgtm

[ronag]

The origin should always be in origDispatchOpts. This seems to be a hack.

[luddd3]

The origin should always be in origDispatchOpts. This seems to be a hack.

I kinda agree, but didn't find where else to put it?

[luddd3]

I made an alternative solution. It currently breaks a test for test/interceptors/retry.js and I'm unsure if it is an actual error or not? Should I just change the expected value?

[metcoder95]

Just adjust the retry tests and should be ok to go.

The change is kind of expected as the origin is being explicitly passed and is the first origin the client is facing

[ronag]

This still looks weird... where/how does the origin get lost?

[luddd3]

The interceptor does only have access to the options called by .request(). The request method has access to all variables in this (e.g Pool) since it is bound to this here:

undici/lib/dispatcher/dispatcher.js

Line 23 in c2933ef

letdispatch=this.dispatch.bind(this)

It could probably be solved in the same manner for the interceptor by changing:

undici/lib/dispatcher/dispatcher.js

Line 34 in c2933ef

dispatch=interceptor(dispatch)

Into:

dispatch=interceptor.bind(this)(dispatch)

or something like this (which is a bit like my original approach):

dispatch=interceptor(dispatch,this)

The last method has the benefit of allowing the interceptors to return arrow functions as well as ordinary functions.

[mcollina]

THis now conflicts, can you rebase?

[ronag]

This is wrong... not sure when I have time to look into it. Christmas...

[metcoder95]

Let me try to check it during the weekend; if there's a workaround, let's take it, otherwise this should be ok for now and we can revisit later so dns is unblocked

[ronag]

The problem here is the assumption that you don't have to pass origin in the request options if used with Client or Pool, which IMHO is incorrect and it's working as intended.

If anything maybe the dns interceptor should check for origin and throw an invalid arg error. Alternatively, if no origin is passed then the dns interceptor is a noop.

[ronag]

diff --git a/lib/interceptor/dns.js b/lib/interceptor/dns.js
index c8d56c2c..c6a1a480 100644
--- a/lib/interceptor/dns.js
+++ b/lib/interceptor/dns.js
@@ -342,6 +342,10 @@ module.exports = interceptorOpts => {
return dispatch => {
return function dnsInterceptor (origDispatchOpts, handler) {
+ if (origDispatchOpts.origin == null) {
+ return dispatch(origDispatchOpts, handler)
+ }
+
const origin =

[mcollina]

lgtm

[luddd3]

The problem here is the assumption that you don't have to pass origin in the request options if used with Client or Pool, which IMHO is incorrect and it's working as intended.

@ronag That was my assumption. Just to be clear, do you think it should be like this?

constclient=newPool('https://google.com',{connections: 10})awaitclient.request({origin: 'https://google.com',// origin must be included in each requestmethod: 'GET',path: '/'})

If that is the case, then I think there are a lot of inconsistencies and contradictions in both the code and documentation.

[ronag]

If that is the case, then I think there are a lot of inconsistencies and contradictions in both the code and documentation.

Possibly. Then let's just make the dns interceptor a noop when origin is missing as I suggested with the patch.

[metcoder95]

I do agree that the documentation has some statements that are either not fully true or confusing for the reader that can lead to a wrong assumption; but that's at an overall take.

The last being said; I'm not 100% in sync with the statement that passing the origin should be mandated on every request call. I'm in sync when use through dynamic dispatchers like Agent or the BalancedPool, but not fully in sync for Client and Pool which are tight to a single origin.

For the latter use-cases, they should have the origin already stick to their state and do not ask the caller to pass it as its goal is to be tight to a single downstream.

I'm ok with the dns interceptor doing a noop if no origin is passed or if it is already an IP address; but the fact that Pool and Client does not forward the origin seems a bit of miss alignment with its purpose

[luddd3]

I think that Client and Pool actually should throw an error when origin is provided to request(), so that the caller notices that it is wrong.

Below is an example of how it works today when different origin is provided to Client during the constructor and request(). The caller might think that the request will be made to http://localhost:2000, which it won't. In this case a thrown error would be much clearer and avoid mistakes. It works similarly for Pool.

import{createServer}from'node:http'import{Client}from'undici'constserver=createServer()server.on('request',(req,res)=>{res.end('hello')})server.listen(0)constclient=newClient(`http://localhost:${server.address().port}`)constresponse=awaitclient.request({method: 'GET',origin: 'http://localhost:2000',path: '/'})console.log(awaitresponse.body.text())// will print 'hello'awaitserver.close()

[luddd3]

I'm ok with the dns interceptor doing a noop if no origin is passed or if it is already an IP address; but the fact that Pool and Client does not forward the origin seems a bit of miss alignment with its purpose

I think that it should throw an error, since it should never be the case that origin is missing. Unless it can get origin directly from Client or Pool via one of the solutions I provided earlier.

[ronag]

I think that Client and Pool actually should throw an error when origin is provided to request(), so that the caller notices that it is wrong.

It should throw when a different origin is provided. Yes.

[luddd3]

It should throw when a different origin is provided. Yes.

I can understand from the viewpoint of backwards-compatibility, but are there other benefits for not throwing every time? I'm afraid that it encourages the wrong behavior to allow an origin, which isn't used,. I have however made a commit which does that.

[mcollina]

lgtm

[ronag]

Still blocking. Please don't override request and instead ignore requests without origin in the dna interceptor

[luddd3]

Still blocking. Please don't override request and instead ignore requests without origin in the dna interceptor

@ronag

1. Why is it better to ignore requests without origin in the dns interceptor instead of throwing an error?
2. Do you mean that I should remove all code I placed in Client and Pool, or just that I shouldn't modify the request options if origin was provided?

[ronag]

The only change here should be a condition in the dns interceptor and a test.

[luddd3]

The only change here should be a condition in the dns interceptor and a test.

So basically remove all changes so far, apply the patch your provided earlier and then write a test?

[luddd3]

IMHO that doesn't solve anything. I as a user would not expect the dns interceptor to be silently bypassed when origin wasn't provided again in my request.

Everything points to origin not being necessary with Client and Pool and it is also not possible to switch origin, so why not override request and make sure that the interceptors get it? Even the examples in the documentation https://undici.nodejs.org/#/docs/api/Client?id=example-client-connect-event shows that request can be called without providing origin again.

[metcoder95]

Gentle ping

[luddd3]

Gentle ping

I don't know how to move forward since none of my proposed solutions have been accepted and I don't think the alternatives are any good. Do you have any ideas?

[metcoder95]

cc: @ronag

[ronag]

I'm not sure what's wrong with my proposal?

Expect origin to be passed to request, if it is passed, make sure it's same as the Pool/Client and if not passed then it's a noop for the dns interceptor.

[luddd3]

I'm not sure what's wrong with my proposal?

Expect origin to be passed to request, if it is passed, make sure it's same as the Pool/Client and if not passed then it's a noop for the dns interceptor.

I think it is wrong that:

1. Client and Pool forces the user to pass origin for each request. It isn't documentet and wouldn't work for different origins. fix: dns interceptor with Pool #3957 (comment)
2. the dns interceptor should be silently bypassed if a different origin is passed. I much rather it throws an error so that the user is informed that there is something wrong.

[luddd3]

Fixed with #5624

[luddd3]

This relates to...

Rationale

The DNS interceptor didn't work with Pool/Client since it didn't get the origin unless included with each request.

Changes

Features

Bug Fixes

• fix dns interceptor with Client/Pool

Breaking Changes and Deprecations

Status

• I have read and agreed to the Developer's Certificate of Origin
• Tested
• Benchmarked (optional)
• Documented
• Review ready
• In review
• Merge ready

[mcollina]

lgtm

[ronag]

The origin should always be in origDispatchOpts. This seems to be a hack.

[luddd3]

The origin should always be in origDispatchOpts. This seems to be a hack.

I kinda agree, but didn't find where else to put it?

[luddd3]

I made an alternative solution. It currently breaks a test for test/interceptors/retry.js and I'm unsure if it is an actual error or not? Should I just change the expected value?

[metcoder95]

Just adjust the retry tests and should be ok to go.

The change is kind of expected as the origin is being explicitly passed and is the first origin the client is facing

[ronag]

This still looks weird... where/how does the origin get lost?

[luddd3]

The interceptor does only have access to the options called by .request(). The request method has access to all variables in this (e.g Pool) since it is bound to this here:

undici/lib/dispatcher/dispatcher.js

Line 23 in c2933ef

letdispatch=this.dispatch.bind(this)

It could probably be solved in the same manner for the interceptor by changing:

undici/lib/dispatcher/dispatcher.js

Line 34 in c2933ef

dispatch=interceptor(dispatch)

Into:

dispatch=interceptor.bind(this)(dispatch)

or something like this (which is a bit like my original approach):

dispatch=interceptor(dispatch,this)

The last method has the benefit of allowing the interceptors to return arrow functions as well as ordinary functions.

[mcollina]

THis now conflicts, can you rebase?

[ronag]

This is wrong... not sure when I have time to look into it. Christmas...

[metcoder95]

Let me try to check it during the weekend; if there's a workaround, let's take it, otherwise this should be ok for now and we can revisit later so dns is unblocked

[ronag]

The problem here is the assumption that you don't have to pass origin in the request options if used with Client or Pool, which IMHO is incorrect and it's working as intended.

If anything maybe the dns interceptor should check for origin and throw an invalid arg error. Alternatively, if no origin is passed then the dns interceptor is a noop.

[ronag]

diff --git a/lib/interceptor/dns.js b/lib/interceptor/dns.js
index c8d56c2c..c6a1a480 100644
--- a/lib/interceptor/dns.js
+++ b/lib/interceptor/dns.js
@@ -342,6 +342,10 @@ module.exports = interceptorOpts => {
return dispatch => {
return function dnsInterceptor (origDispatchOpts, handler) {
+ if (origDispatchOpts.origin == null) {
+ return dispatch(origDispatchOpts, handler)
+ }
+
const origin =

[mcollina]

lgtm

[luddd3]

The problem here is the assumption that you don't have to pass origin in the request options if used with Client or Pool, which IMHO is incorrect and it's working as intended.

@ronag That was my assumption. Just to be clear, do you think it should be like this?

constclient=newPool('https://google.com',{connections: 10})awaitclient.request({origin: 'https://google.com',// origin must be included in each requestmethod: 'GET',path: '/'})

If that is the case, then I think there are a lot of inconsistencies and contradictions in both the code and documentation.

[ronag]

If that is the case, then I think there are a lot of inconsistencies and contradictions in both the code and documentation.

Possibly. Then let's just make the dns interceptor a noop when origin is missing as I suggested with the patch.

[metcoder95]

I do agree that the documentation has some statements that are either not fully true or confusing for the reader that can lead to a wrong assumption; but that's at an overall take.

The last being said; I'm not 100% in sync with the statement that passing the origin should be mandated on every request call. I'm in sync when use through dynamic dispatchers like Agent or the BalancedPool, but not fully in sync for Client and Pool which are tight to a single origin.

For the latter use-cases, they should have the origin already stick to their state and do not ask the caller to pass it as its goal is to be tight to a single downstream.

I'm ok with the dns interceptor doing a noop if no origin is passed or if it is already an IP address; but the fact that Pool and Client does not forward the origin seems a bit of miss alignment with its purpose

[luddd3]

I think that Client and Pool actually should throw an error when origin is provided to request(), so that the caller notices that it is wrong.

Below is an example of how it works today when different origin is provided to Client during the constructor and request(). The caller might think that the request will be made to http://localhost:2000, which it won't. In this case a thrown error would be much clearer and avoid mistakes. It works similarly for Pool.

import{createServer}from'node:http'import{Client}from'undici'constserver=createServer()server.on('request',(req,res)=>{res.end('hello')})server.listen(0)constclient=newClient(`http://localhost:${server.address().port}`)constresponse=awaitclient.request({method: 'GET',origin: 'http://localhost:2000',path: '/'})console.log(awaitresponse.body.text())// will print 'hello'awaitserver.close()

[luddd3]

I'm ok with the dns interceptor doing a noop if no origin is passed or if it is already an IP address; but the fact that Pool and Client does not forward the origin seems a bit of miss alignment with its purpose

I think that it should throw an error, since it should never be the case that origin is missing. Unless it can get origin directly from Client or Pool via one of the solutions I provided earlier.

[ronag]

I think that Client and Pool actually should throw an error when origin is provided to request(), so that the caller notices that it is wrong.

It should throw when a different origin is provided. Yes.

[luddd3]

It should throw when a different origin is provided. Yes.

I can understand from the viewpoint of backwards-compatibility, but are there other benefits for not throwing every time? I'm afraid that it encourages the wrong behavior to allow an origin, which isn't used,. I have however made a commit which does that.

[mcollina]

lgtm

[ronag]

Still blocking. Please don't override request and instead ignore requests without origin in the dna interceptor

[luddd3]

Still blocking. Please don't override request and instead ignore requests without origin in the dna interceptor

@ronag

1. Why is it better to ignore requests without origin in the dns interceptor instead of throwing an error?
2. Do you mean that I should remove all code I placed in Client and Pool, or just that I shouldn't modify the request options if origin was provided?

[ronag]

The only change here should be a condition in the dns interceptor and a test.

[luddd3]

The only change here should be a condition in the dns interceptor and a test.

So basically remove all changes so far, apply the patch your provided earlier and then write a test?

[luddd3]

IMHO that doesn't solve anything. I as a user would not expect the dns interceptor to be silently bypassed when origin wasn't provided again in my request.

Everything points to origin not being necessary with Client and Pool and it is also not possible to switch origin, so why not override request and make sure that the interceptors get it? Even the examples in the documentation https://undici.nodejs.org/#/docs/api/Client?id=example-client-connect-event shows that request can be called without providing origin again.

[metcoder95]

Gentle ping

[luddd3]

Gentle ping

I don't know how to move forward since none of my proposed solutions have been accepted and I don't think the alternatives are any good. Do you have any ideas?

[metcoder95]

cc: @ronag

[ronag]

I'm not sure what's wrong with my proposal?

Expect origin to be passed to request, if it is passed, make sure it's same as the Pool/Client and if not passed then it's a noop for the dns interceptor.

[luddd3]

I'm not sure what's wrong with my proposal?

Expect origin to be passed to request, if it is passed, make sure it's same as the Pool/Client and if not passed then it's a noop for the dns interceptor.

I think it is wrong that:

1. Client and Pool forces the user to pass origin for each request. It isn't documentet and wouldn't work for different origins. fix: dns interceptor with Pool #3957 (comment)
2. the dns interceptor should be silently bypassed if a different origin is passed. I much rather it throws an error so that the user is informed that there is something wrong.

[luddd3]

Fixed with #5624

[luddd3]

This relates to...

Rationale

The DNS interceptor didn't work with Pool/Client since it didn't get the origin unless included with each request.

Changes

Features

Bug Fixes

• fix dns interceptor with Client/Pool

Breaking Changes and Deprecations

Status

• I have read and agreed to the Developer's Certificate of Origin
• Tested
• Benchmarked (optional)
• Documented
• Review ready
• In review
• Merge ready

[mcollina]

lgtm

[ronag]

The origin should always be in origDispatchOpts. This seems to be a hack.

[luddd3]

The origin should always be in origDispatchOpts. This seems to be a hack.

I kinda agree, but didn't find where else to put it?

[luddd3]

I made an alternative solution. It currently breaks a test for test/interceptors/retry.js and I'm unsure if it is an actual error or not? Should I just change the expected value?

[metcoder95]

Just adjust the retry tests and should be ok to go.

The change is kind of expected as the origin is being explicitly passed and is the first origin the client is facing

[ronag]

This still looks weird... where/how does the origin get lost?

[luddd3]

The interceptor does only have access to the options called by .request(). The request method has access to all variables in this (e.g Pool) since it is bound to this here:

undici/lib/dispatcher/dispatcher.js

Line 23 in c2933ef

letdispatch=this.dispatch.bind(this)

It could probably be solved in the same manner for the interceptor by changing:

undici/lib/dispatcher/dispatcher.js

Line 34 in c2933ef

dispatch=interceptor(dispatch)

Into:

dispatch=interceptor.bind(this)(dispatch)

or something like this (which is a bit like my original approach):

dispatch=interceptor(dispatch,this)

The last method has the benefit of allowing the interceptors to return arrow functions as well as ordinary functions.

[mcollina]

THis now conflicts, can you rebase?

[ronag]

This is wrong... not sure when I have time to look into it. Christmas...

[metcoder95]

Let me try to check it during the weekend; if there's a workaround, let's take it, otherwise this should be ok for now and we can revisit later so dns is unblocked

[ronag]

The problem here is the assumption that you don't have to pass origin in the request options if used with Client or Pool, which IMHO is incorrect and it's working as intended.

If anything maybe the dns interceptor should check for origin and throw an invalid arg error. Alternatively, if no origin is passed then the dns interceptor is a noop.

[ronag]

diff --git a/lib/interceptor/dns.js b/lib/interceptor/dns.js
index c8d56c2c..c6a1a480 100644
--- a/lib/interceptor/dns.js
+++ b/lib/interceptor/dns.js
@@ -342,6 +342,10 @@ module.exports = interceptorOpts => {
return dispatch => {
return function dnsInterceptor (origDispatchOpts, handler) {
+ if (origDispatchOpts.origin == null) {
+ return dispatch(origDispatchOpts, handler)
+ }
+
const origin =

[mcollina]

lgtm

[luddd3]

The problem here is the assumption that you don't have to pass origin in the request options if used with Client or Pool, which IMHO is incorrect and it's working as intended.

@ronag That was my assumption. Just to be clear, do you think it should be like this?

constclient=newPool('https://google.com',{connections: 10})awaitclient.request({origin: 'https://google.com',// origin must be included in each requestmethod: 'GET',path: '/'})

If that is the case, then I think there are a lot of inconsistencies and contradictions in both the code and documentation.

[ronag]

If that is the case, then I think there are a lot of inconsistencies and contradictions in both the code and documentation.

Possibly. Then let's just make the dns interceptor a noop when origin is missing as I suggested with the patch.

[metcoder95]

I do agree that the documentation has some statements that are either not fully true or confusing for the reader that can lead to a wrong assumption; but that's at an overall take.

The last being said; I'm not 100% in sync with the statement that passing the origin should be mandated on every request call. I'm in sync when use through dynamic dispatchers like Agent or the BalancedPool, but not fully in sync for Client and Pool which are tight to a single origin.

For the latter use-cases, they should have the origin already stick to their state and do not ask the caller to pass it as its goal is to be tight to a single downstream.

I'm ok with the dns interceptor doing a noop if no origin is passed or if it is already an IP address; but the fact that Pool and Client does not forward the origin seems a bit of miss alignment with its purpose

[luddd3]

I think that Client and Pool actually should throw an error when origin is provided to request(), so that the caller notices that it is wrong.

Below is an example of how it works today when different origin is provided to Client during the constructor and request(). The caller might think that the request will be made to http://localhost:2000, which it won't. In this case a thrown error would be much clearer and avoid mistakes. It works similarly for Pool.

import{createServer}from'node:http'import{Client}from'undici'constserver=createServer()server.on('request',(req,res)=>{res.end('hello')})server.listen(0)constclient=newClient(`http://localhost:${server.address().port}`)constresponse=awaitclient.request({method: 'GET',origin: 'http://localhost:2000',path: '/'})console.log(awaitresponse.body.text())// will print 'hello'awaitserver.close()

[luddd3]

I'm ok with the dns interceptor doing a noop if no origin is passed or if it is already an IP address; but the fact that Pool and Client does not forward the origin seems a bit of miss alignment with its purpose

I think that it should throw an error, since it should never be the case that origin is missing. Unless it can get origin directly from Client or Pool via one of the solutions I provided earlier.

[ronag]

I think that Client and Pool actually should throw an error when origin is provided to request(), so that the caller notices that it is wrong.

It should throw when a different origin is provided. Yes.

[luddd3]

It should throw when a different origin is provided. Yes.

I can understand from the viewpoint of backwards-compatibility, but are there other benefits for not throwing every time? I'm afraid that it encourages the wrong behavior to allow an origin, which isn't used,. I have however made a commit which does that.

[mcollina]

lgtm

[ronag]

Still blocking. Please don't override request and instead ignore requests without origin in the dna interceptor

[luddd3]

Still blocking. Please don't override request and instead ignore requests without origin in the dna interceptor

@ronag

1. Why is it better to ignore requests without origin in the dns interceptor instead of throwing an error?
2. Do you mean that I should remove all code I placed in Client and Pool, or just that I shouldn't modify the request options if origin was provided?

[ronag]

The only change here should be a condition in the dns interceptor and a test.

[luddd3]

The only change here should be a condition in the dns interceptor and a test.

So basically remove all changes so far, apply the patch your provided earlier and then write a test?

[luddd3]

IMHO that doesn't solve anything. I as a user would not expect the dns interceptor to be silently bypassed when origin wasn't provided again in my request.

Everything points to origin not being necessary with Client and Pool and it is also not possible to switch origin, so why not override request and make sure that the interceptors get it? Even the examples in the documentation https://undici.nodejs.org/#/docs/api/Client?id=example-client-connect-event shows that request can be called without providing origin again.

[metcoder95]

Gentle ping

[luddd3]

Gentle ping

I don't know how to move forward since none of my proposed solutions have been accepted and I don't think the alternatives are any good. Do you have any ideas?

[metcoder95]

cc: @ronag

[ronag]

I'm not sure what's wrong with my proposal?

Expect origin to be passed to request, if it is passed, make sure it's same as the Pool/Client and if not passed then it's a noop for the dns interceptor.

[luddd3]

I'm not sure what's wrong with my proposal?

Expect origin to be passed to request, if it is passed, make sure it's same as the Pool/Client and if not passed then it's a noop for the dns interceptor.

I think it is wrong that:

1. Client and Pool forces the user to pass origin for each request. It isn't documentet and wouldn't work for different origins. fix: dns interceptor with Pool #3957 (comment)
2. the dns interceptor should be silently bypassed if a different origin is passed. I much rather it throws an error so that the user is informed that there is something wrong.

[luddd3]

Fixed with #5624

[luddd3]

This relates to...

Rationale

The DNS interceptor didn't work with Pool/Client since it didn't get the origin unless included with each request.

Changes

Features

Bug Fixes

• fix dns interceptor with Client/Pool

Breaking Changes and Deprecations

Status

• I have read and agreed to the Developer's Certificate of Origin
• Tested
• Benchmarked (optional)
• Documented
• Review ready
• In review
• Merge ready

[mcollina]

lgtm

[ronag]

The origin should always be in origDispatchOpts. This seems to be a hack.

[luddd3]

The origin should always be in origDispatchOpts. This seems to be a hack.

I kinda agree, but didn't find where else to put it?

[luddd3]

I made an alternative solution. It currently breaks a test for test/interceptors/retry.js and I'm unsure if it is an actual error or not? Should I just change the expected value?

[metcoder95]

Just adjust the retry tests and should be ok to go.

The change is kind of expected as the origin is being explicitly passed and is the first origin the client is facing

[ronag]

This still looks weird... where/how does the origin get lost?

[luddd3]

The interceptor does only have access to the options called by .request(). The request method has access to all variables in this (e.g Pool) since it is bound to this here:

undici/lib/dispatcher/dispatcher.js

Line 23 in c2933ef

letdispatch=this.dispatch.bind(this)

It could probably be solved in the same manner for the interceptor by changing:

undici/lib/dispatcher/dispatcher.js

Line 34 in c2933ef

dispatch=interceptor(dispatch)

Into:

dispatch=interceptor.bind(this)(dispatch)

or something like this (which is a bit like my original approach):

dispatch=interceptor(dispatch,this)

The last method has the benefit of allowing the interceptors to return arrow functions as well as ordinary functions.

[mcollina]

THis now conflicts, can you rebase?

[ronag]

This is wrong... not sure when I have time to look into it. Christmas...

[metcoder95]

Let me try to check it during the weekend; if there's a workaround, let's take it, otherwise this should be ok for now and we can revisit later so dns is unblocked

[ronag]

The problem here is the assumption that you don't have to pass origin in the request options if used with Client or Pool, which IMHO is incorrect and it's working as intended.

If anything maybe the dns interceptor should check for origin and throw an invalid arg error. Alternatively, if no origin is passed then the dns interceptor is a noop.

[ronag]

diff --git a/lib/interceptor/dns.js b/lib/interceptor/dns.js
index c8d56c2c..c6a1a480 100644
--- a/lib/interceptor/dns.js
+++ b/lib/interceptor/dns.js
@@ -342,6 +342,10 @@ module.exports = interceptorOpts => {
return dispatch => {
return function dnsInterceptor (origDispatchOpts, handler) {
+ if (origDispatchOpts.origin == null) {
+ return dispatch(origDispatchOpts, handler)
+ }
+
const origin =

[mcollina]

lgtm

[luddd3]

The problem here is the assumption that you don't have to pass origin in the request options if used with Client or Pool, which IMHO is incorrect and it's working as intended.

@ronag That was my assumption. Just to be clear, do you think it should be like this?

constclient=newPool('https://google.com',{connections: 10})awaitclient.request({origin: 'https://google.com',// origin must be included in each requestmethod: 'GET',path: '/'})

If that is the case, then I think there are a lot of inconsistencies and contradictions in both the code and documentation.

[ronag]

If that is the case, then I think there are a lot of inconsistencies and contradictions in both the code and documentation.

Possibly. Then let's just make the dns interceptor a noop when origin is missing as I suggested with the patch.

[metcoder95]

I do agree that the documentation has some statements that are either not fully true or confusing for the reader that can lead to a wrong assumption; but that's at an overall take.

The last being said; I'm not 100% in sync with the statement that passing the origin should be mandated on every request call. I'm in sync when use through dynamic dispatchers like Agent or the BalancedPool, but not fully in sync for Client and Pool which are tight to a single origin.

For the latter use-cases, they should have the origin already stick to their state and do not ask the caller to pass it as its goal is to be tight to a single downstream.

I'm ok with the dns interceptor doing a noop if no origin is passed or if it is already an IP address; but the fact that Pool and Client does not forward the origin seems a bit of miss alignment with its purpose

[luddd3]

I think that Client and Pool actually should throw an error when origin is provided to request(), so that the caller notices that it is wrong.

Below is an example of how it works today when different origin is provided to Client during the constructor and request(). The caller might think that the request will be made to http://localhost:2000, which it won't. In this case a thrown error would be much clearer and avoid mistakes. It works similarly for Pool.

import{createServer}from'node:http'import{Client}from'undici'constserver=createServer()server.on('request',(req,res)=>{res.end('hello')})server.listen(0)constclient=newClient(`http://localhost:${server.address().port}`)constresponse=awaitclient.request({method: 'GET',origin: 'http://localhost:2000',path: '/'})console.log(awaitresponse.body.text())// will print 'hello'awaitserver.close()

[luddd3]

I'm ok with the dns interceptor doing a noop if no origin is passed or if it is already an IP address; but the fact that Pool and Client does not forward the origin seems a bit of miss alignment with its purpose

I think that it should throw an error, since it should never be the case that origin is missing. Unless it can get origin directly from Client or Pool via one of the solutions I provided earlier.

[ronag]

I think that Client and Pool actually should throw an error when origin is provided to request(), so that the caller notices that it is wrong.

It should throw when a different origin is provided. Yes.

[luddd3]

It should throw when a different origin is provided. Yes.

I can understand from the viewpoint of backwards-compatibility, but are there other benefits for not throwing every time? I'm afraid that it encourages the wrong behavior to allow an origin, which isn't used,. I have however made a commit which does that.

[mcollina]

lgtm

[ronag]

Still blocking. Please don't override request and instead ignore requests without origin in the dna interceptor

[luddd3]

Still blocking. Please don't override request and instead ignore requests without origin in the dna interceptor

@ronag

1. Why is it better to ignore requests without origin in the dns interceptor instead of throwing an error?
2. Do you mean that I should remove all code I placed in Client and Pool, or just that I shouldn't modify the request options if origin was provided?

[ronag]

The only change here should be a condition in the dns interceptor and a test.

[luddd3]

The only change here should be a condition in the dns interceptor and a test.

So basically remove all changes so far, apply the patch your provided earlier and then write a test?

[luddd3]

IMHO that doesn't solve anything. I as a user would not expect the dns interceptor to be silently bypassed when origin wasn't provided again in my request.

Everything points to origin not being necessary with Client and Pool and it is also not possible to switch origin, so why not override request and make sure that the interceptors get it? Even the examples in the documentation https://undici.nodejs.org/#/docs/api/Client?id=example-client-connect-event shows that request can be called without providing origin again.

[metcoder95]

Gentle ping

[luddd3]

Gentle ping

I don't know how to move forward since none of my proposed solutions have been accepted and I don't think the alternatives are any good. Do you have any ideas?

[metcoder95]

cc: @ronag

[ronag]

I'm not sure what's wrong with my proposal?

Expect origin to be passed to request, if it is passed, make sure it's same as the Pool/Client and if not passed then it's a noop for the dns interceptor.

[luddd3]

I'm not sure what's wrong with my proposal?

Expect origin to be passed to request, if it is passed, make sure it's same as the Pool/Client and if not passed then it's a noop for the dns interceptor.

I think it is wrong that:

1. Client and Pool forces the user to pass origin for each request. It isn't documentet and wouldn't work for different origins. fix: dns interceptor with Pool #3957 (comment)
2. the dns interceptor should be silently bypassed if a different origin is passed. I much rather it throws an error so that the user is informed that there is something wrong.

[luddd3]

Fixed with #5624

[luddd3]

This relates to...

Rationale

The DNS interceptor didn't work with Pool/Client since it didn't get the origin unless included with each request.

Changes

Features

Bug Fixes

• fix dns interceptor with Client/Pool

Breaking Changes and Deprecations

Status

• I have read and agreed to the Developer's Certificate of Origin
• Tested
• Benchmarked (optional)
• Documented
• Review ready
• In review
• Merge ready

[mcollina]

lgtm

[ronag]

The origin should always be in origDispatchOpts. This seems to be a hack.

[luddd3]

The origin should always be in origDispatchOpts. This seems to be a hack.

I kinda agree, but didn't find where else to put it?

[luddd3]

I made an alternative solution. It currently breaks a test for test/interceptors/retry.js and I'm unsure if it is an actual error or not? Should I just change the expected value?

[metcoder95]

Just adjust the retry tests and should be ok to go.

The change is kind of expected as the origin is being explicitly passed and is the first origin the client is facing

[ronag]

This still looks weird... where/how does the origin get lost?

[luddd3]

The interceptor does only have access to the options called by .request(). The request method has access to all variables in this (e.g Pool) since it is bound to this here:

undici/lib/dispatcher/dispatcher.js

Line 23 in c2933ef

letdispatch=this.dispatch.bind(this)

It could probably be solved in the same manner for the interceptor by changing:

undici/lib/dispatcher/dispatcher.js

Line 34 in c2933ef

dispatch=interceptor(dispatch)

Into:

dispatch=interceptor.bind(this)(dispatch)

or something like this (which is a bit like my original approach):

dispatch=interceptor(dispatch,this)

The last method has the benefit of allowing the interceptors to return arrow functions as well as ordinary functions.

[mcollina]

THis now conflicts, can you rebase?

[ronag]

This is wrong... not sure when I have time to look into it. Christmas...

[metcoder95]

Let me try to check it during the weekend; if there's a workaround, let's take it, otherwise this should be ok for now and we can revisit later so dns is unblocked

[ronag]

The problem here is the assumption that you don't have to pass origin in the request options if used with Client or Pool, which IMHO is incorrect and it's working as intended.

If anything maybe the dns interceptor should check for origin and throw an invalid arg error. Alternatively, if no origin is passed then the dns interceptor is a noop.

[ronag]

diff --git a/lib/interceptor/dns.js b/lib/interceptor/dns.js
index c8d56c2c..c6a1a480 100644
--- a/lib/interceptor/dns.js
+++ b/lib/interceptor/dns.js
@@ -342,6 +342,10 @@ module.exports = interceptorOpts => {
return dispatch => {
return function dnsInterceptor (origDispatchOpts, handler) {
+ if (origDispatchOpts.origin == null) {
+ return dispatch(origDispatchOpts, handler)
+ }
+
const origin =

[mcollina]

lgtm

[luddd3]

The problem here is the assumption that you don't have to pass origin in the request options if used with Client or Pool, which IMHO is incorrect and it's working as intended.

@ronag That was my assumption. Just to be clear, do you think it should be like this?

constclient=newPool('https://google.com',{connections: 10})awaitclient.request({origin: 'https://google.com',// origin must be included in each requestmethod: 'GET',path: '/'})

If that is the case, then I think there are a lot of inconsistencies and contradictions in both the code and documentation.

[ronag]

If that is the case, then I think there are a lot of inconsistencies and contradictions in both the code and documentation.

Possibly. Then let's just make the dns interceptor a noop when origin is missing as I suggested with the patch.

[metcoder95]

I do agree that the documentation has some statements that are either not fully true or confusing for the reader that can lead to a wrong assumption; but that's at an overall take.

The last being said; I'm not 100% in sync with the statement that passing the origin should be mandated on every request call. I'm in sync when use through dynamic dispatchers like Agent or the BalancedPool, but not fully in sync for Client and Pool which are tight to a single origin.

For the latter use-cases, they should have the origin already stick to their state and do not ask the caller to pass it as its goal is to be tight to a single downstream.

I'm ok with the dns interceptor doing a noop if no origin is passed or if it is already an IP address; but the fact that Pool and Client does not forward the origin seems a bit of miss alignment with its purpose

[luddd3]

I think that Client and Pool actually should throw an error when origin is provided to request(), so that the caller notices that it is wrong.

Below is an example of how it works today when different origin is provided to Client during the constructor and request(). The caller might think that the request will be made to http://localhost:2000, which it won't. In this case a thrown error would be much clearer and avoid mistakes. It works similarly for Pool.

import{createServer}from'node:http'import{Client}from'undici'constserver=createServer()server.on('request',(req,res)=>{res.end('hello')})server.listen(0)constclient=newClient(`http://localhost:${server.address().port}`)constresponse=awaitclient.request({method: 'GET',origin: 'http://localhost:2000',path: '/'})console.log(awaitresponse.body.text())// will print 'hello'awaitserver.close()

[luddd3]

I'm ok with the dns interceptor doing a noop if no origin is passed or if it is already an IP address; but the fact that Pool and Client does not forward the origin seems a bit of miss alignment with its purpose

I think that it should throw an error, since it should never be the case that origin is missing. Unless it can get origin directly from Client or Pool via one of the solutions I provided earlier.

[ronag]

I think that Client and Pool actually should throw an error when origin is provided to request(), so that the caller notices that it is wrong.

It should throw when a different origin is provided. Yes.

[luddd3]

It should throw when a different origin is provided. Yes.

I can understand from the viewpoint of backwards-compatibility, but are there other benefits for not throwing every time? I'm afraid that it encourages the wrong behavior to allow an origin, which isn't used,. I have however made a commit which does that.

[mcollina]

lgtm

[ronag]

Still blocking. Please don't override request and instead ignore requests without origin in the dna interceptor

[luddd3]

Still blocking. Please don't override request and instead ignore requests without origin in the dna interceptor

@ronag

1. Why is it better to ignore requests without origin in the dns interceptor instead of throwing an error?
2. Do you mean that I should remove all code I placed in Client and Pool, or just that I shouldn't modify the request options if origin was provided?

[ronag]

The only change here should be a condition in the dns interceptor and a test.

[luddd3]

The only change here should be a condition in the dns interceptor and a test.

So basically remove all changes so far, apply the patch your provided earlier and then write a test?

[luddd3]

IMHO that doesn't solve anything. I as a user would not expect the dns interceptor to be silently bypassed when origin wasn't provided again in my request.

Everything points to origin not being necessary with Client and Pool and it is also not possible to switch origin, so why not override request and make sure that the interceptors get it? Even the examples in the documentation https://undici.nodejs.org/#/docs/api/Client?id=example-client-connect-event shows that request can be called without providing origin again.

[metcoder95]

Gentle ping

[luddd3]

Gentle ping

I don't know how to move forward since none of my proposed solutions have been accepted and I don't think the alternatives are any good. Do you have any ideas?

[metcoder95]

cc: @ronag

[ronag]

I'm not sure what's wrong with my proposal?

Expect origin to be passed to request, if it is passed, make sure it's same as the Pool/Client and if not passed then it's a noop for the dns interceptor.

[luddd3]

I'm not sure what's wrong with my proposal?

Expect origin to be passed to request, if it is passed, make sure it's same as the Pool/Client and if not passed then it's a noop for the dns interceptor.

I think it is wrong that:

1. Client and Pool forces the user to pass origin for each request. It isn't documentet and wouldn't work for different origins. fix: dns interceptor with Pool #3957 (comment)
2. the dns interceptor should be silently bypassed if a different origin is passed. I much rather it throws an error so that the user is informed that there is something wrong.

[luddd3]

Fixed with #5624

[luddd3]

This relates to...

Rationale

The DNS interceptor didn't work with Pool/Client since it didn't get the origin unless included with each request.

Changes

Features

Bug Fixes

• fix dns interceptor with Client/Pool

Breaking Changes and Deprecations

Status

• I have read and agreed to the Developer's Certificate of Origin
• Tested
• Benchmarked (optional)
• Documented
• Review ready
• In review
• Merge ready

[mcollina]

lgtm

[ronag]

The origin should always be in origDispatchOpts. This seems to be a hack.

[luddd3]

The origin should always be in origDispatchOpts. This seems to be a hack.

I kinda agree, but didn't find where else to put it?

[luddd3]

I made an alternative solution. It currently breaks a test for test/interceptors/retry.js and I'm unsure if it is an actual error or not? Should I just change the expected value?

[metcoder95]

Just adjust the retry tests and should be ok to go.

The change is kind of expected as the origin is being explicitly passed and is the first origin the client is facing

[ronag]

This still looks weird... where/how does the origin get lost?

[luddd3]

The interceptor does only have access to the options called by .request(). The request method has access to all variables in this (e.g Pool) since it is bound to this here:

undici/lib/dispatcher/dispatcher.js

Line 23 in c2933ef

letdispatch=this.dispatch.bind(this)

It could probably be solved in the same manner for the interceptor by changing:

undici/lib/dispatcher/dispatcher.js

Line 34 in c2933ef

dispatch=interceptor(dispatch)

Into:

dispatch=interceptor.bind(this)(dispatch)

or something like this (which is a bit like my original approach):

dispatch=interceptor(dispatch,this)

The last method has the benefit of allowing the interceptors to return arrow functions as well as ordinary functions.

[mcollina]

THis now conflicts, can you rebase?

[ronag]

This is wrong... not sure when I have time to look into it. Christmas...

[metcoder95]

Let me try to check it during the weekend; if there's a workaround, let's take it, otherwise this should be ok for now and we can revisit later so dns is unblocked

[ronag]

The problem here is the assumption that you don't have to pass origin in the request options if used with Client or Pool, which IMHO is incorrect and it's working as intended.

If anything maybe the dns interceptor should check for origin and throw an invalid arg error. Alternatively, if no origin is passed then the dns interceptor is a noop.

[ronag]

diff --git a/lib/interceptor/dns.js b/lib/interceptor/dns.js
index c8d56c2c..c6a1a480 100644
--- a/lib/interceptor/dns.js
+++ b/lib/interceptor/dns.js
@@ -342,6 +342,10 @@ module.exports = interceptorOpts => {
return dispatch => {
return function dnsInterceptor (origDispatchOpts, handler) {
+ if (origDispatchOpts.origin == null) {
+ return dispatch(origDispatchOpts, handler)
+ }
+
const origin =

[mcollina]

lgtm

[luddd3]

The problem here is the assumption that you don't have to pass origin in the request options if used with Client or Pool, which IMHO is incorrect and it's working as intended.

@ronag That was my assumption. Just to be clear, do you think it should be like this?

constclient=newPool('https://google.com',{connections: 10})awaitclient.request({origin: 'https://google.com',// origin must be included in each requestmethod: 'GET',path: '/'})

If that is the case, then I think there are a lot of inconsistencies and contradictions in both the code and documentation.

[ronag]

If that is the case, then I think there are a lot of inconsistencies and contradictions in both the code and documentation.

Possibly. Then let's just make the dns interceptor a noop when origin is missing as I suggested with the patch.

[metcoder95]

I do agree that the documentation has some statements that are either not fully true or confusing for the reader that can lead to a wrong assumption; but that's at an overall take.

The last being said; I'm not 100% in sync with the statement that passing the origin should be mandated on every request call. I'm in sync when use through dynamic dispatchers like Agent or the BalancedPool, but not fully in sync for Client and Pool which are tight to a single origin.

For the latter use-cases, they should have the origin already stick to their state and do not ask the caller to pass it as its goal is to be tight to a single downstream.

I'm ok with the dns interceptor doing a noop if no origin is passed or if it is already an IP address; but the fact that Pool and Client does not forward the origin seems a bit of miss alignment with its purpose

[luddd3]

I think that Client and Pool actually should throw an error when origin is provided to request(), so that the caller notices that it is wrong.

Below is an example of how it works today when different origin is provided to Client during the constructor and request(). The caller might think that the request will be made to http://localhost:2000, which it won't. In this case a thrown error would be much clearer and avoid mistakes. It works similarly for Pool.

import{createServer}from'node:http'import{Client}from'undici'constserver=createServer()server.on('request',(req,res)=>{res.end('hello')})server.listen(0)constclient=newClient(`http://localhost:${server.address().port}`)constresponse=awaitclient.request({method: 'GET',origin: 'http://localhost:2000',path: '/'})console.log(awaitresponse.body.text())// will print 'hello'awaitserver.close()

[luddd3]

I'm ok with the dns interceptor doing a noop if no origin is passed or if it is already an IP address; but the fact that Pool and Client does not forward the origin seems a bit of miss alignment with its purpose

I think that it should throw an error, since it should never be the case that origin is missing. Unless it can get origin directly from Client or Pool via one of the solutions I provided earlier.

[ronag]

I think that Client and Pool actually should throw an error when origin is provided to request(), so that the caller notices that it is wrong.

It should throw when a different origin is provided. Yes.

[luddd3]

It should throw when a different origin is provided. Yes.

I can understand from the viewpoint of backwards-compatibility, but are there other benefits for not throwing every time? I'm afraid that it encourages the wrong behavior to allow an origin, which isn't used,. I have however made a commit which does that.

[mcollina]

lgtm

[ronag]

Still blocking. Please don't override request and instead ignore requests without origin in the dna interceptor

[luddd3]

Still blocking. Please don't override request and instead ignore requests without origin in the dna interceptor

@ronag

1. Why is it better to ignore requests without origin in the dns interceptor instead of throwing an error?
2. Do you mean that I should remove all code I placed in Client and Pool, or just that I shouldn't modify the request options if origin was provided?

[ronag]

The only change here should be a condition in the dns interceptor and a test.

[luddd3]

The only change here should be a condition in the dns interceptor and a test.

So basically remove all changes so far, apply the patch your provided earlier and then write a test?

[luddd3]

IMHO that doesn't solve anything. I as a user would not expect the dns interceptor to be silently bypassed when origin wasn't provided again in my request.

Everything points to origin not being necessary with Client and Pool and it is also not possible to switch origin, so why not override request and make sure that the interceptors get it? Even the examples in the documentation https://undici.nodejs.org/#/docs/api/Client?id=example-client-connect-event shows that request can be called without providing origin again.

[metcoder95]

Gentle ping

[luddd3]

Gentle ping

I don't know how to move forward since none of my proposed solutions have been accepted and I don't think the alternatives are any good. Do you have any ideas?

[metcoder95]

cc: @ronag

[ronag]

I'm not sure what's wrong with my proposal?

Expect origin to be passed to request, if it is passed, make sure it's same as the Pool/Client and if not passed then it's a noop for the dns interceptor.

[luddd3]

I'm not sure what's wrong with my proposal?

Expect origin to be passed to request, if it is passed, make sure it's same as the Pool/Client and if not passed then it's a noop for the dns interceptor.

I think it is wrong that:

1. Client and Pool forces the user to pass origin for each request. It isn't documentet and wouldn't work for different origins. fix: dns interceptor with Pool #3957 (comment)
2. the dns interceptor should be silently bypassed if a different origin is passed. I much rather it throws an error so that the user is informed that there is something wrong.

[luddd3]

Fixed with #5624