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

grpc-js: Use a more structured representation of URIs internally #1364

Merged
merged 5 commits into from
Apr 20, 2020

Conversation

murgatroid99
Copy link
Member

@murgatroid99 murgatroid99 commented Apr 16, 2020

This change is a result of a few things:

  1. When working on the proxy code, I noticed that there were a few pieces of code that performed overlapping but not identical operations related to parsing targets/addresses.
  2. DNS_REGEX Can Hang Node Process #1359 was filed, and I realized that the easy fix would make the already unwieldy regexes in the DNS resolver even worse.
  3. I found the core URI parsing code and I realized that a structured representation of URIs could solve both problems.

The new parseUri function is based on grpc_uri_parse, except that I don't handle query parameters or fragments because they don't seem to be useful anywhere.

The new splitHostPort function is based on DoSplitHostPort.

One major thing to note: I realized after implementing this that for a target without a real scheme like localhost:80, a completely valid parsing of that target could have scheme='localhost' and path='80', but that is not useful. So, if a target has an unknown scheme, the resolver uses the whole target as the path.

This fixes #1359.

@ThWoywod
Copy link

I use this Library behind a HTTP Proxy and figure out that currently it does not work after this Refactoring!

With this commit (238a91c) everything works well.

Its probably an error with the HttpProxy request over here

const request = http.request(options);

The request (with the master branch) contains the following options:

{ 
     method: 'CONNECT',
     path: '443',
     host: '2a00:1450:400e:808::200a',
     port: 443 
}

The request (before this refactoring (commit: 238a91c)) contains:

{ 
     method: 'CONNECT',
     host: '192.168.1.130',  // my http proxy IP
     port: 3128, // my http proxy port
     path: 'dialogflow.googleapis.com:443'
} 

@murgatroid99
Copy link
Member Author

@ThWoywod Thank you for the information. I think I fixed the problem in #1375.

@murgatroid99
Copy link
Member Author

I think I fixed that in version 1.0.1

@ThWoywod
Copy link

@murgatroid99 Thank you for the fast fix! 👍
Do you know when version 1.0.1 will be released?

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.

DNS_REGEX Can Hang Node Process
3 participants