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

Capture and log errors from promise chains #995

Merged
merged 1 commit into from May 27, 2013

Conversation

Projects
None yet
6 participants
@ahawkins
Contributor

ahawkins commented May 24, 2013

This fixes this problem:

promise.then(function() {
  throw "Fail here for whatever reason"
});

The thrown error never sees the light of day because it's treated as
promise rejection and not a failure. I encournted this problem when
ED was erroring internally trying to sideload association. The
serializer raised an exception (inside a then callback) that was
swallowed and ruined my god damn day. Here's the offending code:

return this.ajax(this.buildURL(root), "GET",{
      data: this.sinceQuery(since)
    }).then(function(json) {
      // errrors from `didFindAll` are ignored. This is horrible.
      Ember.run(adapter,'didFindAll', store, type, json);
    })

EDIT: I want to make something clear. It's these sort of issues that prevent me recommending ember-data to anyone. This seriously crippled @dagda1's and I's brains. The time required to simply figure out what is wrong with JSON parsing is unacceptable. Seriously, who can use this library? /rant.

@machty

View changes

Show outdated Hide outdated packages/ember-data/lib/adapters/rest_adapter.js Outdated
@dagda1

This comment has been minimized.

Show comment
Hide comment
@dagda1

dagda1 May 24, 2013

If you look at didFindAll of the current RESTAdapter for example:

  findAll: function(store, type, since) {
    var root, adapter;

    root = this.rootForType(type);
    adapter = this;

    return this.ajax(this.buildURL(root), "GET",{
      data: this.sinceQuery(since)
    }).then(function(json) {
      Ember.run(adapter,'didFindAll', store, type, json);
    });
  },

There is absolutely no way of knowing when any error happens, e.g. a key is wrongly named in the incoming JSON or maybe I have an attribute wrongly named in the ED class as all errors are swallowed.

The didFindXXX methods are an integration point with an external service (more than likely). Exceptions will happen here and having all exceptions swallowed renders these methods at best useless and at worst damaging because I might be new to ember data and have no idea why things are not working.

dagda1 commented May 24, 2013

If you look at didFindAll of the current RESTAdapter for example:

  findAll: function(store, type, since) {
    var root, adapter;

    root = this.rootForType(type);
    adapter = this;

    return this.ajax(this.buildURL(root), "GET",{
      data: this.sinceQuery(since)
    }).then(function(json) {
      Ember.run(adapter,'didFindAll', store, type, json);
    });
  },

There is absolutely no way of knowing when any error happens, e.g. a key is wrongly named in the incoming JSON or maybe I have an attribute wrongly named in the ED class as all errors are swallowed.

The didFindXXX methods are an integration point with an external service (more than likely). Exceptions will happen here and having all exceptions swallowed renders these methods at best useless and at worst damaging because I might be new to ember data and have no idea why things are not working.

@tomdale

This comment has been minimized.

Show comment
Hide comment
@tomdale

tomdale May 24, 2013

Member

Shouldn't we re-raise the exception instead of logging it to the console?

Member

tomdale commented May 24, 2013

Shouldn't we re-raise the exception instead of logging it to the console?

@dagda1

This comment has been minimized.

Show comment
Hide comment
@dagda1

dagda1 May 24, 2013

I don't think that you can re-raise it because the promises library will swallow it and look for a reject handler.

dagda1 commented May 24, 2013

I don't think that you can re-raise it because the promises library will swallow it and look for a reject handler.

@teddyzeenny

This comment has been minimized.

Show comment
Hide comment
@teddyzeenny

teddyzeenny May 24, 2013

Contributor

I don't think we should log it. IMO the error should be re-thrown (asynchronously to escape the promise).

.then(null, function(error) {
  setTimeout(function() {
    throw error;
  });
});
Contributor

teddyzeenny commented May 24, 2013

I don't think we should log it. IMO the error should be re-thrown (asynchronously to escape the promise).

.then(null, function(error) {
  setTimeout(function() {
    throw error;
  });
});
@dagda1

This comment has been minimized.

Show comment
Hide comment
@dagda1

dagda1 commented May 24, 2013

@ahawkins

This comment has been minimized.

Show comment
Hide comment
@ahawkins

ahawkins May 24, 2013

Contributor

@teddyzeenny does this provide a useful stacktrace?

Contributor

ahawkins commented May 24, 2013

@teddyzeenny does this provide a useful stacktrace?

@teddyzeenny

This comment has been minimized.

Show comment
Hide comment
@teddyzeenny

teddyzeenny May 24, 2013

Contributor

@ahawkins no :( but the stacktrace was probably already gone due to the async nature of promises.

Contributor

teddyzeenny commented May 24, 2013

@ahawkins no :( but the stacktrace was probably already gone due to the async nature of promises.

@ahawkins

This comment has been minimized.

Show comment
Hide comment
@ahawkins

ahawkins May 24, 2013

Contributor

@teddyzeeny the stacktrace is somewhat useful with the console.error
method. It does not include the method inside then but it does point to
the method where the promise is declared.

On Fri, May 24, 2013 at 7:05 PM, Teddy Zeenny notifications@github.comwrote:

@ahawkins https://github.com/ahawkins no :( but the stacktrace was
probably already gone due to the async nature of promises.


Reply to this email directly or view it on GitHubhttps://github.com//pull/995#issuecomment-18417489
.

Contributor

ahawkins commented May 24, 2013

@teddyzeeny the stacktrace is somewhat useful with the console.error
method. It does not include the method inside then but it does point to
the method where the promise is declared.

On Fri, May 24, 2013 at 7:05 PM, Teddy Zeenny notifications@github.comwrote:

@ahawkins https://github.com/ahawkins no :( but the stacktrace was
probably already gone due to the async nature of promises.


Reply to this email directly or view it on GitHubhttps://github.com//pull/995#issuecomment-18417489
.

@dagda1

This comment has been minimized.

Show comment
Hide comment
@dagda1

dagda1 May 24, 2013

I have to ask the question then, are promises a bad fit for these methods?

dagda1 commented May 24, 2013

I have to ask the question then, are promises a bad fit for these methods?

@teddyzeenny

This comment has been minimized.

Show comment
Hide comment
@teddyzeenny

teddyzeenny May 25, 2013

Contributor

@ahawkins you are right, it's better, but not perfect. We must find a way to prevent promises from handling Ember.assert errors. That would be the best solution.

Contributor

teddyzeenny commented May 25, 2013

@ahawkins you are right, it's better, but not perfect. We must find a way to prevent promises from handling Ember.assert errors. That would be the best solution.

@ahawkins

This comment has been minimized.

Show comment
Hide comment
@ahawkins

ahawkins May 26, 2013

Contributor

I have to ask the question then, are promises a bad fit for these methods?

I don't think so. I think they are the right design choice. A simple error handler solves all the things we ran into.

Contributor

ahawkins commented May 26, 2013

I have to ask the question then, are promises a bad fit for these methods?

I don't think so. I think they are the right design choice. A simple error handler solves all the things we ran into.

@ahawkins

This comment has been minimized.

Show comment
Hide comment
@ahawkins

ahawkins May 26, 2013

Contributor

@stefanpenner thoughts on this? This is a pretty big deal.

Contributor

ahawkins commented May 26, 2013

@stefanpenner thoughts on this? This is a pretty big deal.

@ghost ghost assigned stefanpenner May 27, 2013

@stefanpenner

This comment has been minimized.

Show comment
Hide comment
@stefanpenner

stefanpenner May 27, 2013

Member

a rejectionHandler with console.error (if supported) @ https://github.com/emberjs/data/blob/master/packages/ember-data/lib/adapters/rest_adapter.js#L333 might be the correct path.

[edit] looks like we also need this handler in the other places identified by @ahawkins

Now as for the rejectionHandler itself, in addition to providing useful feedback (via log, or assert, or setTimeout re-throw), it must re-throw the error, or the promise becomes fulfilled, and downstream listeners believe the operation has been performed successfully.

Something like, would be my first pass at it:

function rejectionHandler(reason) {
  Ember.logger.error(reason);
  throw reason;
}

return Ember.RSVP.resolve(jQuery.ajax(hash)).then(null, rejectionHandler);

@ahawkins I also think this is supper important, give my suggestion a try. If it seems to do the trick, add some test coverage. (ensuring both console.error and the inline re-throw occur.)

Member

stefanpenner commented May 27, 2013

a rejectionHandler with console.error (if supported) @ https://github.com/emberjs/data/blob/master/packages/ember-data/lib/adapters/rest_adapter.js#L333 might be the correct path.

[edit] looks like we also need this handler in the other places identified by @ahawkins

Now as for the rejectionHandler itself, in addition to providing useful feedback (via log, or assert, or setTimeout re-throw), it must re-throw the error, or the promise becomes fulfilled, and downstream listeners believe the operation has been performed successfully.

Something like, would be my first pass at it:

function rejectionHandler(reason) {
  Ember.logger.error(reason);
  throw reason;
}

return Ember.RSVP.resolve(jQuery.ajax(hash)).then(null, rejectionHandler);

@ahawkins I also think this is supper important, give my suggestion a try. If it seems to do the trick, add some test coverage. (ensuring both console.error and the inline re-throw occur.)

twinturbo
Capture and log errors from promise chains
The thrown error never sees the light of day because it's treated as
promise rejection and **not** a failure. I encournted this problem when
ED was erroring _internally_ trying to sideload association. The
serializer raised an exception (inside a then callback) that was
swallowed.

stefanpenner added a commit that referenced this pull request May 27, 2013

Merge pull request #995 from ahawkins/log-promise-errors
Capture and log errors from promise chains

@stefanpenner stefanpenner merged commit bf8609d into emberjs:master May 27, 2013

1 check was pending

default The Travis CI build is in progress
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment