Skip to content

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

Merged
murgatroid99 merged 5 commits intogrpc:masterfrom
murgatroid99:grpc-js_uri_parsing
Apr 20, 2020
Merged

grpc-js: Use a more structured representation of URIs internally#1364
murgatroid99 merged 5 commits intogrpc:masterfrom
murgatroid99:grpc-js_uri_parsing

Conversation

@murgatroid99
Copy link
Copy Markdown
Member

@murgatroid99 murgatroid99 commented Apr 16, 2020 •
edited
Loading

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.

@murgatroid99 murgatroid99 merged commit ae61562 into grpc:master Apr 20, 2020
@mrfelton mrfelton mentioned this pull request Apr 21, 2020
Merged
@ThWoywod
Copy link
Copy Markdown

ThWoywod commented Apr 21, 2020

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

grpc-node/packages/grpc-js/src/http_proxy.ts

Line 194 in ae61562

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
Copy Markdown
Member Author

murgatroid99 commented Apr 21, 2020

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

@murgatroid99
Copy link
Copy Markdown
Member Author

murgatroid99 commented Apr 21, 2020

I think I fixed that in version 1.0.1

@ThWoywod
Copy link
Copy Markdown

ThWoywod commented Apr 22, 2020

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

@badsyntax badsyntax mentioned this pull request Apr 23, 2020
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer

@nicolasnoble nicolasnoble nicolasnoble approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

DNS_REGEX Can Hang Node Process

3 participants

@murgatroid99 @ThWoywod @nicolasnoble