onHandleError is sometimes triggered for handled Bluebird rejections

[joews]

• Zone.js 0.8.26
• Bluebird 3.5.0
• Chrome 67

onHandledError is sometimes called for handled rejections when using the Zone.js Bluebird patch.

In this example, onHandleError is called despite catch. The catch callback itself is not called.

const Bluebird = require("bluebird");
require("zone.js");
require("zone.js/dist/zone-bluebird");
Zone[Zone["__symbol__"]("bluebird")](Bluebird);

const zone = Zone.current
  .fork({
    name: "testErrorHandling",
    onHandleError(a, b, c, err) {
      console.log("onHandleError caught: " + err);
    }
  })

zone.runGuarded(() => {
    return Bluebird.resolve()
      .then(() => {
        throw new Error("test error");
      })
      .catch(() => {
        // never invoked
        console.log("caught");
      });
  });

If the runGuarded callback calls Bluebird.reject instead, we get the expected behaviour: the catch callback is called, onHandleError is not.

zone.runGuarded(() => {
    return Bluebird.reject(new Error("test error"))
      .catch(() => {
        // this time it is invoked
        console.log("caught");
      });
  });

If global.Promise is patched before the Zone.js Bluebird patch we get the expected behaviour in both examples, whether we use Bluebird or Promise.

const Bluebird = require("bluebird");
global.Promise = Bluebird;

require("zone.js");
require("zone.js/dist/zone-bluebird");
Zone[Zone["__symbol__"]("bluebird")](Bluebird);

If global.Promise is patched after the Zone.js Bluebird patch, the Promise.reject example invokes the catch callback as expected. However, in the Promise.resolve example, even the .then callback is not called. No error is thrown, and onHandleError is not invoked.

const Bluebird = require("bluebird");

require("zone.js");
require("zone.js/dist/zone-bluebird");
Zone[Zone["__symbol__"]("bluebird")](Bluebird);
global.Promise = Bluebird;

// same ZoneSpec as above...

zone.runGuarded(() => {
  return Promise.resolve()
    .then(() => {
      // never executed!     
      throw new Error("never happens")
    })
    .catch(() => {
        // never called
     })
});

I think this last one may be incorrect usage, but I couldn't confirm that from the docs.

[JiaLiPassion]

@joews, thank you for posting the issue, I will check it.

[JiaLiPassion]

@joews, The issue is being fixed here #1114.

And I will clarify the usage.

Setup

const Bluebird = require("bluebird");
require("zone.js");
require("zone.js/dist/zone-bluebird");
Zone[Zone["__symbol__"]("bluebird")](Bluebird);

You don't need to set global.Promise = Bluebird, the zone-bluebird already did this.

ZoneSpec

Basically, onHandleError will not be called unless unhandledRejection occurs (Rejected Promise without catch).

// same ZoneSpec as above...
const zone = Zone.current
  .fork({
    name: "testErrorHandling",
    onHandleError(a, b, c, err) {
      console.log("onHandleError caught: ");
      return true;
    }
  })

zone.runGuarded(() => {
  return Promise.resolve()
    .then(() => {
      // never executed!
      throw new Error("never happens")
    })
});

catch

catch should always be called with a rejected Promise.

You can use the attached zone.js.zip to test. (replace node_modules/zone.js/dist)
zone.js.zip

[joews]

@JiaLiPassion thanks for the response!

The patch fixes my problem - onHandledError is not called for the handled rejection in my first example. Awesome, thank you! Do you know when we may see a release with this fix?

Also thanks for the clarification on window.Promise. I saw that you don't need to patch manually, but after the catch/onHandledError issue I started playing around to see what changed.

I saw that is still possible to break Promise by manually patching:

const Bluebird = require("bluebird");
require("zone.js");
require("zone.js/dist/zone-bluebird");

Zone[Zone["__symbol__"]("bluebird")](Bluebird);

console.log(Promise === Bluebird); // logs `true`

// If the next line is uncommented, neither `then` callback is run.
// window.Promise = Bluebird;

Promise.resolve()
  .then(() => {
    console.log("never called (Promise)");
  });

Bluebird.resolve()
  .then(() => {
    console.log("never called (Bluebird)");
  });

This comes from the set trap here:

zone.js/lib/common/promise.ts

Line 411 in bf88c34

if (NewNativePromise === ZoneAwarePromise) {

☝️ this check fails after the Bluebird patch, and api.setNativePromise somehow isn't compatible with the already-patched Bluebird.

It's an edge case, but maybe Zone should be robust against this re-assignment. I can see it both ways - if you don't agree, i'll raise a PR to make this explicit in the non-standard API docs.

Thanks again for the fast reply, and for your great work on Zone.js!

[JiaLiPassion]

@joews, thank you for the confirmation, and I will check the re-assignment case, it should work...

[JiaLiPassion]

@joews, in nodejs, the code you post will just work, to use it in the browser, it needs additional setup, I will add it to document.

<html>
  <head>
    <script src="./node_modules/bluebird/js/browser/bluebird.js"></script>
    <script src="./node_modules/zone.js/dist/zone.js"></script>
    <script src="./node_modules/zone.js/dist/zone-bluebird.js"></script>
    <script>
      var Bluebird = window['P'];
      Zone[Zone["__symbol__"]("bluebird")](Bluebird);

// console.log(Promise === Bluebird); // logs `true`

// If the next line is uncommented, neither `then` callback is run.
window.Promise = Bluebird;

Promise.resolve()
  .then(() => {
    console.log("never called (Promise)");
  });

Bluebird.resolve()
  .then(() => {
    console.log("never called (Bluebird)");
  });
    </script>
  </head>
</html>

[joews]

@JiaLiPassion thanks for your help with this issue.

Do you know when we may see a release with this fix?

[JiaLiPassion]

@joews, I am not sure the release date, please wait for a while, I can create a branch for a temp release if it will help.

[joews]

@JiaLiPassion that would be really useful, thanks.

[JiaLiPassion]

@joews, sure, you can use this one in package.json

"zone.js": "git://github.com/JiaLiPassion/zone.js#bluebird-then-release"