luciotato / waitfor

Sequential programming for node.js, end of callback hell / pyramid of doom
MIT License
531 stars 29 forks source link

Is it possible to test for fiber.run() exception condition in advance? #14

Closed BobDickinson closed 9 years ago

BobDickinson commented 10 years ago

I have two modules that implement the same async interface. One of them talks to the cloud and is truly async. The other one talks to the local file system and is currently implemented using the sync fs functions. The latter functions all call their callbacks before the functions exit, as they don't have any async waits. In my current development environment (Node Tools for Visual Studio) this causes a debug break in the code below (from waitfor.js - applyAndWait):

    try { fiber.run(); }   //resume fiber
    catch(e){ // ignore error: "Fiber is already running" (when callback is called before async function returns)
        //console.log(e.stack);
        if (e.message === "This Fiber is already running") null;
        else throw e;
    }; 

This code works fine in production (exception gets caught, error message matches the test, and it continues), but in development it triggers an exception break every time this condition happens, which is pretty annoying. I'm wondering if there is a way to test for the condition that causes the exception so as to avoid the fiber.run() call altogether (and the exception break, in my case).

BobDickinson commented 10 years ago

OK, actually I found that there was a setting in NTVS (turned on by default, for Node errors only) to break on all exceptions instead of just unhandled exceptions. So I was able to turn that off and this problem is gone.

That being said, it is a somewhat normal condition, at least in my usage, and it would be cleaner to avoid the exception if possible (if it's straightforward - if not, just close this).

luciotato commented 10 years ago

Maybe you already know this, but the best option for you is to use fs async functions in the module handling local files, so both modules are truly async. wait.for allows it, but it is not expected for an "async" function to callback before return. If you can make both modules truly async, you'll avoid problems further ahead.

BobDickinson commented 10 years ago

Yeah, I will probably change the sync files to async, but it still seems like a very normal condition (to call the callback before function exit). For example, returning an error from an async function via the callback before getting to the async part (maybe parameter validation or something) is pretty common. Using an exception for a "normal" condition is not great programming practice, and I'm not thrilled about a string match on the error message as the way this condition is identified. I'd feel a lot better about this code if it had a more reasonable method of dealing with this condition (predicting it in advance and not generating the exception in the first place).

luciotato commented 10 years ago

That's a fair point.

I've pushed a small change to github to handle that condition better. https://github.com/luciotato/waitfor/commit/5e1b08e411066846ead03f6b453d74a57762b4c0

Please test it. Tell me if it it works. if it's OK I'll bump up the version and update the npm package.

BobDickinson commented 10 years ago

The change looks good to me. I tested it in my project without any issues (though admittedly I don't use it in too many places, and the places I do are not well covered by unit tests). But I did run it through the basic async scenarios.