catch all unhandled promise rejection for http browser io handler

[pyu10055]

fixes tensorflow/tfjs#992
This change would catch all unhandled promise rejection and create meaningful error messages for network errors or content type errors.
It adds 'Accept' header to ensure the server side would fail if the content is not available instead of redirect to a standard page.
It also verify the return content type header matches the request.

---

For repository owners only:

Please remember to apply all applicable tags to your pull request.
Tags: FEATURE, BREAKING, BUG, PERF, DEV, DOC, SECURITY

For more info see: https://github.com/tensorflow/tfjs/blob/master/DEVELOPMENT.md

---

This change is 

[nsthorat]

Reviewable status: 0 of 1 approvals obtained (waiting on @pyu10055 and @nsthorat)

---

src/io/browser_http.ts, line 187 at r1 (raw file):

      return {modelTopology, weightSpecs, weightData};
    } catch (error) {
      throw new Error(`Failed to load binary model: ${error}`);

this is still a pretty blanket exception, could we get more granular with the requests in here? here and below

[nsthorat]

Hey can we prioritize this? A few people are asking about it.

[nsthorat]

One more thing we should do here is when the server gives an HTML response for shards (e.g. not octet-stream). We get these errors about the number of bytes of the response being off, but we should error with the status code of the server.

[nsthorat]

Here is an example:

[nsthorat]

Here are some proposals for error messages:

• Request to http://.... failed with status code 500 (this would happen for each type of request)
• Request to http://... expected a mime type octet-stream but got ... (this would happen for all requests)
• Request to http:// returned xyz bytes but expected abc bytes.

Here are some proposals from Daniel: tensorflow/tfjs#924 (comment)

Thanks Ping!

[pyu10055]

We should use Accept header for all requests, and if the server cannot fulfill it would throw 406 error.
This should cover the mime-type error and wrong size issue. I think we should handle the http error in a general way, not to push bunch custom error interpretation logic.

Reviewable status: 0 of 1 approvals obtained (waiting on @pyu10055)

[nsthorat]

Reviewable status: 0 of 1 approvals obtained (waiting on @pyu10055)

---

src/io/browser_http.ts, line 187 at r1 (raw file):

Previously, nsthorat (Nikhil Thorat) wrote…

this is still a pretty blanket exception, could we get more granular with the requests in here? here and below

Instead of nesting errors by catching internal errors and rebubbling them let's just throw a useful exception at the lowest level, does that make sense? Here and elsewhere we do the nesting of exceptions, bubbling, catching, and rethrowing

---

src/io/browser_http.ts, line 173 at r2 (raw file):

    const contentType = response.headers.get('content-type');
    if (!contentType || contentType.indexOf(target) === -1) {
      throw new Error(`Wrong content type (${contentType}) for ${

How about:
url: the full url
type: "model topology" | "weights file"

Request to ${url} for ${type} failed. Expected content type: ${expected} but got ${actual}

[pyu10055]

Addressed the comments and fixed test for node.js. PTAL

Reviewable status: 0 of 1 approvals obtained (waiting on @pyu10055)

[pyu10055]

Reviewable status: 0 of 1 approvals obtained

---

src/io/browser_http.ts, line 187 at r1 (raw file):

Previously, nsthorat (Nikhil Thorat) wrote…

Instead of nesting errors by catching internal errors and rebubbling them let's just throw a useful exception at the lowest level, does that make sense? Here and elsewhere we do the nesting of exceptions, bubbling, catching, and rethrowing

wrapped the fectchFunc to catch the promise rejection, removed the nested catch rethrow.

[nsthorat]

Can you update the PR description? It looks like this just checks the content type now, not try catching and throwing all errors.

Reviewable status: 0 of 1 approvals obtained (waiting on @pyu10055)

---

src/io/browser_http.ts, line 62 at r3 (raw file):

    this.fetchFunc = (path: string, requestInits: RequestInit) => {
      // tslint:disable-next-line:no-any
      return fetchFunc(path, requestInits).catch((error: any) => {

you can just not type error and you won't need this no-any

---

src/io/browser_http_test.ts, line 86 at r3 (raw file):

      [filename: string]: {
        data: string|Float32Array|Int32Array|ArrayBuffer|Uint8Array|Uint16Array,
        contentType: string

isnt this always octet-stream? why pass this?

---

src/io/browser_http_test.ts, line 117 at r3 (raw file):

  afterAll(() => {
    // tslint:disable-next-line:no-any
    delete (global as any).fetch;

wont this completely delete fetch? why not restore the fetch from before you overwrote it

---

src/io/browser_http_test.ts, line 799 at r3 (raw file):

        done.fail(
            'Loading with fetch rejection ' +
            'succeeded unexpectedly.');

i think this fits on the previous line

---

src/io/browser_http_test.ts, line 856 at r3 (raw file):

                .toEqual(floatData);
            expect(Object.keys(requestInits).length).toEqual(3);
            expect(requestInits['./model.pb'].headers['Accept'])

you can use .get() here and everywhere else

---

src/io/browser_http_test.ts, line 1209 at r3 (raw file):

    it('with wrong manifest content type leads to error',
       async (done: DoneFn) => {

no need to type done here (it's messing up the indentation)

---

src/io/browser_http_test.ts, line 1305 at r3 (raw file):

             .catch(err => {
               expect(err.message)
                   .toMatch(/Expected content type application\/octet-stream/);

instead of just matching the beginning can you assert the entirety of the error message?

---

src/io/weights_loader.ts, line 44 at r3 (raw file):

  const headers = requestOptions.headers || {};
  // tslint:disable-next-line:no-any
  (headers as any)['Accept'] = 'application/octet-stream';

you use this in multiple places, pull it to a constant

---

src/io/weights_loader.ts, line 44 at r3 (raw file):

  const headers = requestOptions.headers || {};
  // tslint:disable-next-line:no-any
  (headers as any)['Accept'] = 'application/octet-stream';

I think you can use this method: myHeaders.append('Content-Type', 'application/octet-stream');

---

src/io/weights_loader.ts, line 53 at r3 (raw file):

  const badContentType = responses.filter(response => {
    const contentType = response.headers.get('content-type');

for consistency make this capitalized (even though it's case insensitive). Also would be good to put this in a constant (and with the .get() below too)

---

src/io/weights_loader.ts, line 64 at r3 (raw file):

                    ` Expected content type application/octet-stream but got ${
                            resp.headers.get('content-type')}.`)
            .join(', '));

this is going to display quite weird (comma separated sentences) -- have you actually run this to see the failure?

---

src/io/weights_loader_test.ts, line 30 at r3 (raw file):

      return new Response(
          fileBufferMap[path],
          {headers: {'content-type': 'application/octet-stream'}});

capitalize for consistency

[pyu10055]

done, thanks.

Reviewable status: 0 of 1 approvals obtained (waiting on @nsthorat)

---

src/io/browser_http.ts, line 62 at r3 (raw file):

Previously, nsthorat (Nikhil Thorat) wrote…

you can just not type error and you won't need this no-any

the fetchFunc is just a Function, its return value is not typed.

---

src/io/browser_http_test.ts, line 86 at r3 (raw file):

Previously, nsthorat (Nikhil Thorat) wrote…

isnt this always octet-stream? why pass this?

this is a helper method for setup response for fake fetch, the input data can be for the model json or different type of weight date.

---

src/io/browser_http_test.ts, line 117 at r3 (raw file):

Previously, nsthorat (Nikhil Thorat) wrote…

wont this completely delete fetch? why not restore the fetch from before you overwrote it

this is previous code, I assume it works because it added to global first, and it is doing it on global instead of window.

---

src/io/browser_http_test.ts, line 856 at r3 (raw file):

expect(requestInits['./model.pb'].headers['Accept'])

the headers can be of Object for node env.

---

src/io/weights_loader.ts, line 44 at r3 (raw file):

Previously, nsthorat (Nikhil Thorat) wrote…

I think you can use this method: myHeaders.append('Content-Type', 'application/octet-stream');

the header can be of object type for node env.

---

src/io/weights_loader.ts, line 64 at r3 (raw file):

Previously, nsthorat (Nikhil Thorat) wrote…

this is going to display quite weird (comma separated sentences) -- have you actually run this to see the failure?

use '\n' instead of ', ' for concatenation.

[nsthorat]

Reviewable status: 0 of 1 approvals obtained (waiting on @pyu10055 and @nsthorat)

---

src/io/browser_http.ts, line 62 at r3 (raw file):

Previously, pyu10055 (Ping Yu) wrote…

the fetchFunc is just a Function, its return value is not typed.

I'm talking about the argument "error".

---

src/io/browser_http_test.ts, line 86 at r3 (raw file):

Previously, pyu10055 (Ping Yu) wrote…

this is a helper method for setup response for fake fetch, the input data can be for the model json or different type of weight date.

ok just default the contentType to octet-stream then so it's not scattered everywhere. Override it where it's incorrect.

---

src/io/browser_http_test.ts, line 117 at r3 (raw file):

Previously, pyu10055 (Ping Yu) wrote…

this is previous code, I assume it works because it added to global first, and it is doing it on global instead of window.

Let's say global.fetch was there before. Then you reset it, then delete it. Now this test has side effects. You should hold onto fetch before overriding then reset it. You can't guarantee fetch is not set on global.

---

src/io/weights_loader.ts, line 44 at r3 (raw file):

Previously, pyu10055 (Ping Yu) wrote…

the header can be of object type for node env.

We should probably add a TypeScript type for this then.

---

src/io/weights_loader.ts, line 64 at r3 (raw file):

Previously, pyu10055 (Ping Yu) wrote…

use '\n' instead of ', ' for concatenation.

I still see ", ".

[pyu10055]

Reviewable status: 0 of 1 approvals obtained (waiting on @nsthorat)

---

src/io/browser_http.ts, line 62 at r3 (raw file):

Previously, nsthorat (Nikhil Thorat) wrote…

I'm talking about the argument "error".

without any, ts complains "Parameter 'error' implicitly has an 'any' type."

---

src/io/browser_http_test.ts, line 86 at r3 (raw file):

Previously, nsthorat (Nikhil Thorat) wrote…

ok just default the contentType to octet-stream then so it's not scattered everywhere. Override it where it's incorrect.

this is a type def, not sure how to set default value?

---

src/io/browser_http_test.ts, line 117 at r3 (raw file):

Previously, nsthorat (Nikhil Thorat) wrote…

Let's say global.fetch was there before. Then you reset it, then delete it. Now this test has side effects. You should hold onto fetch before overriding then reset it. You can't guarantee fetch is not set on global.

saved the original fetch

---

src/io/weights_loader.ts, line 44 at r3 (raw file):

Previously, nsthorat (Nikhil Thorat) wrote…

We should probably add a TypeScript type for this then.

unfortunately, ts thinks the headers is Headers type, but in node there is no Headers object, otherwise, we can create a new Header object here to get rid of any casting.

---

src/io/weights_loader.ts, line 64 at r3 (raw file):

Previously, nsthorat (Nikhil Thorat) wrote…

I still see ", ".

missed the changes

[nsthorat]

Reviewable status: 0 of 1 approvals obtained (waiting on @nsthorat and @pyu10055)

---

src/io/browser_http.ts, line 178 at r4 (raw file):

    if (!contentType || contentType.indexOf(type) === -1) {
      throw new Error(`Request to ${response.url} for ${
          target} failed. Expected content type ${type}) but got ${

remove the paren after ${type}

---

src/io/browser_http.ts, line 188 at r4 (raw file):

        this.path[1], this.addAcceptHeader('application/json'));
    this.verifyContentType(
        manifestPromise, 'weight manifest', 'application/json');

weights manifest

---

src/io/browser_http_test.ts, line 785 at r4 (raw file):

                .toEqual(
                    'Request to path1/model.json for model topology failed. ' +
                    'Expected content type application/json) ' +

why is there a ) here?

---

src/io/weights_loader.ts, line 44 at r3 (raw file):

Previously, pyu10055 (Ping Yu) wrote…

unfortunately, ts thinks the headers is Headers type, but in node there is no Headers object, otherwise, we can create a new Header object here to get rid of any casting.

cast the Header type as your own type that has Accept on it

[nsthorat]

Can you also update the PR description / title?