AndreasMadsen / trace

Creates super long stack traces
https://trace.js.org
MIT License
194 stars 18 forks source link

Support Bluebird coroutines #6

Closed samcday closed 8 years ago

samcday commented 10 years ago

First up, really love this library + stack-chain! Awesome stuff. I investigated using it in an app I have, but unfortunately I ran into some issues. Here's my problem.

I'm not sure how familiar you are with ES6 generators when used with a promise library that supports them (q / bluebird / others). Basically, instead of CPS-style spaghetti or noisy promise handling code, you can do this:

var promise = require("bluebird");

var gogo = Promise.coroutine(function*() {
    yield someMethodThatReturnsAPromise();
    yield someOtherMethodThatAlsoReturnsAPromise();
    throw new Exception("Omg awesome!");
});

process.nextTick(gogo);

Essentially, it makes async code look synchronous again, and you can actually use try/catch to handle errors that come back from async oeprations. Awesome-sauce.

The problem is that this library follows all of these yielded promises through async ops and back again. As a result, the stack trace for the snippet of code above actually includes stack frames for every single yielded promise. This means that instead of seeing a clean stacktrace from when the co-routine was called up until the error was thrown, we instead see the calls to someMethodThatReturnsAPromise and someOtherMethodThatAlsoReturnsAPromise + any async operations they went through in the trace.

I tried to do some hacky insanity with trace. I added a mark + rewind feature that would snapshot the stack, and then reset the callsites back to the saved ones after each yielded promise was resolved, but there was a bunch of corner cases and it felt really ... wrong. I was wondering, since you wrote this library and have obviously thought through this stuff alot, if you had any ideas about how to handle this?

If this all sounds incoherent, let me know and I can craft up a proper test case for you to play with.

AndreasMadsen commented 10 years ago

Hi, thanks for showing your interest. I did notice your fork and spied a bit on your code.

I'm definitely not going too"`support bluebird" specifically, but if there truly is something "wrong" in general then of cause that needs to be addressed.

I'm not sure how familiar you are with ES6 generators

I'm definitely not familiar with it, I've only heard of it and have some weak idea about what its for. If I where to rewrite your example using callbacks it would look like this, right?

process.nextTick(function () {
  someMethod(function () {
    someOtherMethod(function () {
      throw new Exception("Omg awesome!");
    });
  });
});

and we would get a stack trace containing all the call sites from the Exception call to the internal init call, including those from someMethod and someOtherMethod. This is not wired its exactly what we want. However in your yield case the code looks sync and you would want only call sites, as if it where sync, right?

Well I'm not sure its a good idea in general, as it would hide what is actually happening and "what is actually happening" is exactly what we want the stack to show.

What I however do recognize as a problem, is the super long stack traces ( module description :p ) you will quickly get after a few yields or callbacks. This can be solved by a --stack-trace-limit n flag when running node, but in the end we have no idea how many call sites are between the error and the cause. Thus we will pick a high number and get a very slow and nasty error tracer (could be optimized, as the current implementation is naive).

What I hope to be able to do instead is to somehow limit the stack trace, not to a specific number of call sites but to a line in the code. Like:

process.nextTick(function () {
  someMethod(function cb() {
    trace.from(cb);
    someOtherMethod(function () {
      throw new Exception("Omg awesome!");
    });
  });
});

the stack trace would then not show the someMethod and node internal call sites before, only those between someOtherMethod and new Exception. I suppose you could do something similar in promise terms, like:

var gogo = Promise.coroutine(function*() {
    promise = someMethodThatReturnsAPromise();
    yield promise;
    trace.from(promise);
    yield someOtherMethodThatAlsoReturnsAPromise();
    throw new Exception("Omg awesome!");
});

It would surly not the the same as what you suggest (joining two stack frames) but if possible at all (limited by v8 API) it would be simpler to implement. And as you have discovered whatever you want to implement will need to be deadly simple, since it will end up getting complicated anyway. (It took me two days just to feel comfortable about those 60 lines and even after that, there where still issues :) ).

samcday commented 10 years ago

@AndreasMadsen yeah my fork is a mess and is the result of committing insanity that I implemented at 4:30am looking at this stuff too much :)

Your analogy to traditional callback code is a little off. My original example would look like this with Node callback style:

var gogo = function(cb) {
    someMethodThatReturnsAPromise(function(err) {
        if (err) return cb(err);

        someOtherMethodThatAlsoReturnsAPromise(function(err) {
            if (err) return cb(err);
            return cb(new Exception("Omg awesome!"));
        });
    });
};

The difference here is I'd definitely expect to be told the callsites for the invocations of someMethodThatReturnsAPromise and someOtherMethodThatAlsoReturnsAPromise, because of the very nature of CPS. However if I'm working with generator-based promises, I'd expect to throw away the callsites after my yielded promises have resolved, because errors from these calls can be propagated through the suspended generators (using generator.throw()).

The title of my issue is a little misleading, and was the first thing that popped into my mind so that I could open a dialogue regarding this. I apologise! Basically I guess what I'm looking for is similar to your proposal, but instead of throwing away all callsites since a call to trace.from, I'd rather be able to mark + rewind.

This way I can say "mark the callsite chain I'm currently at when I call mark. If I choose to ask you to, please throw away all frames collected between when I marked my position and when I ask you to rewind". Does this make sense?

AndreasMadsen commented 10 years ago

It makes sense, but sounds academic. If you are able to place a mark then you already know the error is after the mark thus you don't need the call sites leading up to this and in any case you could just call console.trace.

So I really think a from method is enough. Coming to think about it a very simple implementation that I believe could work, would be to have an API like:

var gogo = Promise.coroutine(function*() {
    yield someMethodThatReturnsAPromise();
    yield someOtherMethodThatAlsoReturnsAPromise();

    trace.from(function () {
      throw new Exception("Omg awesome!");
    });
});

from would then just call the callback synchronously, thus creating a callsite there is within trace.js, this call site can then be used to remove all the previous call sites.

AndreasMadsen commented 8 years ago

I think we can finally close this after https://github.com/AndreasMadsen/async-hook/pull/2. It may not be exactly what you wanted, but I think what you want is best provided by a third party module that uses stack-chain.