Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

core/request.js: http proxies support #446

Closed
wants to merge 1 commit into from
Closed

core/request.js: http proxies support #446

wants to merge 1 commit into from

Conversation

sergeysedoy97
Copy link

To support http proxies path must accept url.

e.g.

GET http://example.com/ HTTP/1.1

To support http proxies path must accept url.
@ronag
Copy link
Member

ronag commented Oct 5, 2020

I'm not sure I understand why?

@sergeysedoy97
Copy link
Author

I'm not sure I understand why?

Thanks this commit it will be able to use undici with http proxies.

e.g.

const { body } = await new undici.Client('http://127.0.0.1:3128').request({
  method: 'GET',
  path: 'http://site-that-need-proxy.domain/some-path',
})

There is http://127.0.0.1:3128 an http proxy server.

@ronag
Copy link
Member

ronag commented Oct 5, 2020

That doesn't make sense to me... @szmarczak maybe you can help me understand?

@sergeysedoy97
Copy link
Author

That doesn't make sense to me... @szmarczak maybe you can help me understand?

Okay, do you know the way how to use undici with proxy now?

@ronag
Copy link
Member

ronag commented Oct 5, 2020

Okay, do you know the way how to use undici with proxy now?

No because I don't understand what you are trying to do. I suspect you are looking for CONNECT followed by a regular request like we are discussing here #427.

I don't see how your proposed change here would work.

@sergeysedoy97
Copy link
Author

Okay, do you know the way how to use undici with proxy now?

No because I don't understand what you are trying to do. I suspect you are looking for CONNECT followed by a regular request like we are discussing here #427.

I don't see how your proposed change here would work.

It works, I don't need to modify connect mechanics because I don't need to keep connection with proxy (tunneling).

@ronag
Copy link
Member

ronag commented Oct 5, 2020

It works, I don't need to modify connect mechanics because I don't need to keep connection with proxy (tunneling).

Can you add a test?

@sergeysedoy97
Copy link
Author

It works, I don't need to modify connect mechanics because I don't need to keep connection with proxy (tunneling).

Can you add a test?

Of course

@szmarczak
Copy link
Member

That doesn't make sense to me... @szmarczak maybe you can help me understand?

There are different methods to proxy the requests. The most popular is the CONNECT method. But there are also web proxies for HTTP/1.1: https://en.wikipedia.org/wiki/Proxy_server#Web_proxy_servers

@szmarczak
Copy link
Member

It's almost the same as mirroring but you accept an absolute URL instead.

@@ -23,8 +23,8 @@ class Request {
upgrade,
requestTimeout
}, handler) {
if (typeof path !== 'string' || path[0] !== '/') {
throw new InvalidArgumentError('path must be a valid path')
if (typeof path !== 'string') {
Copy link
Member

@ronag ronag Oct 6, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we have a regex that does a more strict check?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep,

let isURL = false;
try {
  new URL(path);
  isURL = true;
} catch {}
console.log(isURL);

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if (typeof path !== 'string' || (path[0] !== '/' && isURL === false))

@ronag
Copy link
Member

ronag commented Oct 10, 2020

@sergeysedoy97 @szmarczak Would either of you like to finish this PR?

@ronag
Copy link
Member

ronag commented Nov 2, 2020

@sergeysedoy97 @szmarczak any of you still interested to continue this PR?

@szmarczak
Copy link
Member

Sure. Will do this now.

@szmarczak
Copy link
Member

test/tls.js fails randomly
szm@solus ~/Desktop/undici $ npm test

> undici@2.1.0 test /home/szm/Desktop/undici
> tap test/*.js --no-coverage && jest test/jest/test

 PASS  test/client-abort.js 4 OK 54.391ms
 PASS  test/abort-controller.js 19 OK 4s
 PASS  test/abort-event-emitter.js 20 OK 4s
 PASS  test/client-dispatch.js 32 OK 4s
 PASS  test/client-connect.js 27 OK 4s
 PASS  test/async_hooks.js 40 OK 4s
 PASS  test/client-errors.js 137 OK 4s
 PASS  test/client-reconnect.js 5 OK 24.904ms
 PASS  test/client-request.js 7 OK 28.229ms
 PASS  test/client.js 1094 OK 4s
 PASS  test/client-pipeline.js 63 OK 4s
 PASS  test/client-upgrade.js 33 OK 4s
 PASS  test/client-stream.js 67 OK 4s
 PASS  test/client-write-max-listeners.js 32 OK 4s
 PASS  test/client-pipelining.js 198 OK 4s
 PASS  test/close-and-destroy.js 46 OK 4s
 PASS  test/client-keep-alive.js 21 OK 8s
 PASS  test/get-head-body.js 15 OK 46.089ms
 PASS  test/headers-as-array.js 3 OK 49.423ms
 PASS  test/invalid-headers.js 10 OK 9.431ms
 PASS  test/fixed-queue.js 2059 OK 170.211ms
 PASS  test/pipeline-pipelining.js 23 OK 27.758ms
 PASS  test/content-length.js 18 OK 5s
 PASS  test/request-timeout.js 30 OK 74.57ms
 PASS  test/parser-issues.js 2 OK 2s
 PASS  test/socket-handle-error.js 4 OK 126.087ms
 PASS  test/esm-wrapper.js 13 OK 4s
 PASS  test/http-100.js 6 OK 4s
 PASS  test/https.js 6 OK 4s
 PASS  test/max-headers.js 3 OK 4s
 PASS  test/trailers.js 4 OK 19.901ms
 PASS  test/promises.js 66 OK 4s
 PASS  test/pool.js 1030 OK 4s
 PASS  test/validations.js 10 OK 14.243ms
 PASS  test/socket-back-pressure.js 3 OK 5s
 PASS  test/socket-timeout.js 11 OK 5s
 PASS  test/stream-compat.js 4 OK 4s
 PASS  test/tls-session-reuse.js 14 OK 4s
 PASS  test/unix.js 20 OK 4s
 FAIL  test/tls.js
 ✖ timeout!

  test: tls get 1
  signal: SIGTERM
  requests:
    - type: TCPConnectWrap
  handles:
    - type: TLSSocket
      events:
        - close
        - end
        - newListener
        - connect
        - secure
        - session
        - secureConnect
        - error
  expired: TAP
  stack: >
    process.emit (node_modules/source-map-support/source-map-support.js:495:21)

    process.domainProcessEmit [as emit] (node_modules/async-hook-domain/index.js:137:26)

 FAIL  test/tls.js 1 failed of 1 30s
 ✖ timeout!


                         
  🌈 SUMMARY RESULTS 🌈  
                         

 FAIL  test/tls.js 1 failed of 1 30s
 ✖ timeout!

Suites:   1 failed, 39 passed, 40 of 40 completed
Asserts:  1 failed, 5199 passed, of 5200
Time:     43s
npm ERR! Test failed.  See above for more details.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants