Skip to content
This repository has been archived by the owner on Oct 3, 2023. It is now read-only.

Fix a TypeError of url.parse #640

Merged

Conversation

StoneDot
Copy link
Contributor

This PR fixes TypeError occurring when http2.connect is called with url.URL.
It is permitted according to official document.

A following code reproduces the error.

Source:

const tracing = require('@opencensus/nodejs');
const zipkin = require('@opencensus/exporter-zipkin');

const tracer = tracing.start({samplingRate: 1.0}).tracer;

const http2 = require('http2');

tracer.registerSpanEventListener(new zipkin.ZipkinTraceExporter({
    url: 'http://localhost:9411/api/v2/spans',
    serviceName: 'node.js-open-census-bug'
}));

const url = new URL('/index.html', 'https://example.com/');
const client = http2.connect(url);

const {
  HTTP2_HEADER_STATUS
} = http2.constants;
const headers = {};
const req = client.request(headers);
req.on('response', headers => {
  console.log(headers[HTTP2_HEADER_STATUS]);
  req.on('data', chunk => console.log(chunk));
  req.on('end', () => process.exit());
});

Result:

internal/validators.js:125
    throw new ERR_INVALID_ARG_TYPE(name, 'string', value);
    ^

TypeError [ERR_INVALID_ARG_TYPE]: The "url" argument must be of type string. Received type object
    at validateString (internal/validators.js:125:11)
    at Url.parse (url.js:151:3)
    at Object.urlParse [as parse] (url.js:146:13)
    at ClientHttp2Stream.request.on (/Users/h-goto/projects/opencensus-bug-report/node_modules/@opencensus/instrumentation-http2/build/src/http2.js:107:34)
    at ClientHttp2Stream.contextWrapper (/Users/h-goto/projects/opencensus-bug-report/node_modules/@opencensus/core/build/src/internal/cls-ah.js:78:28)
    at ClientHttp2Stream.emit (events.js:202:15)
    at endReadableNT (_stream_readable.js:1132:12)
    at processTicksAndRejections (internal/process/next_tick.js:76:17)

@googlebot
Copy link

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here with @googlebot I signed it!) and we'll verify it.


What to do if you already signed the CLA

Individual signers
Corporate signers

ℹ️ Googlers: Go here for more info.

@@ -178,7 +178,10 @@ export class Http2Plugin extends HttpPlugin {
const userAgent =
headers['user-agent'] || headers['User-Agent'] || null;

const host = url.parse(authority).host;
const host = (authority instanceof url.URL
Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks @StoneDot for the fix! Would you be willing to add a unit test for this?

Copy link
Member

Choose a reason for hiding this comment

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

+1, also update the CHANGELOG.md

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've written the tests for this PR, but it hasn't worked correctly because of another bug (See #641).

Could I tweak the tests to pass?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Also I've updated CHANGELOG.md at #de29571

@mayurkale22 mayurkale22 added this to the Release 0.0.18 milestone Aug 30, 2019
@StoneDot
Copy link
Contributor Author

StoneDot commented Sep 5, 2019

Currently I'm preparing to contract CLA as a company (Because this contribution was created on work time). It will take more time to sign a CLA. I am sorry for being late.

@googlebot
Copy link

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

@googlebot googlebot added cla: yes and removed cla: no labels Sep 9, 2019
@StoneDot
Copy link
Contributor Author

StoneDot commented Sep 9, 2019

Rebased to include #650

@StoneDot
Copy link
Contributor Author

StoneDot commented Sep 9, 2019

Could you wait merge? (It seems Googlebot doesn't work correctly for me.)
I have not agreed CLA as a company yet.

@mayurkale22 mayurkale22 merged commit 77ecbf0 into census-instrumentation:master Sep 9, 2019
@mayurkale22
Copy link
Member

@StoneDot Let us know if you are waiting for the next release (0. 0.18) with this fix.

@StoneDot
Copy link
Contributor Author

@mayurkale22 Thanks! But I'm not in a rush.
However, I'd like to get the new release if #647 is merged (sorry for my slow work).

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

Successfully merging this pull request may close these issues.

4 participants