Make uncaught exceptions visible on the online validator about bids-validator HOT 4 CLOSED

I had a look at this with @rwblair but it turns out that simply using try/catch does not work because of the asynchronous nature of JS exceptions (see https://www.joyent.com/developers/node/design/errors#fn:1) @constellates do you have any ideas?

from bids-validator.

Similar to what you've found I don't believe there are any simple global ways to handle exceptions in asynchronous environments.

I think we just need to do a better job managing exceptions at a more granular level.

Sync

I believe all of our individual validators are or can be synchronous. So those can could be wrapped in a try/catch/throw block. If we catch any issues we can bubble them immediately to the output.
This includes:

• validators/bval.js
• validators/bvec.js
• validators/json.js
• validators/nii.js
• validators/session.js
• validators/tsv.js

Some of these like bval.js & bvec.js are trivial enough that it may be overkill but it could be a good strategy for the others.

Async

Most if not all of our asynchronous functionality is tied up in file reading and this is one area that we have quite a bit of exception handling. If we find or create other areas that require async exception handling we can looks to utils/files.js for examples of how those can be managed with callbacks or promises.

API Change

We also need to change the core api slightly to provide a way to expose these exceptions. The most straightforward change would be to change this.

validate.BIDS(directory, options, function (errors, warnings) {
    // log errors
    console.log(errors);
    // log warnings
    console.log(warnings);
}); 

To this common pattern where we first return any errors/exceptions with the validation process and then the errors and warnings wrapped in a response object.

validate.BIDS(directory, options, function (error, response) {
    // log exceptions
    console.log(error);

    // log errors
    console.log(response.errors);
    // log warnings
    console.log(response.warnings);
}); 

Alternatively we could add a promise library if a promise style API would be preferred.

validate.BIDS(directory, options).then(function (error, response) {
    // log errors
    console.log(response.errors);
    // log warnings
}).catch(function (error) {
    // log exceptions
    console.log(error);
});

Other issues

The other related issue we've run into recently is when there are no exceptions thrown anywhere, but the validator returns so many results that the web app crashes trying to render them all. We should definitely be conscious of how this could happen when designing new checks, but how do you feel about applying the same default issue limiting to the web interfaces like we do the the CLI. So we'll only render 10 files per issue and if there are more you would have to click to view more?

from bids-validator.

Sorry for getting back to you so late:

• I like the idea of turning everything into a sync, it will not only allow us to do simple try catch, but also make it easier for non node.js savvy people to contribute to the project
• I don't quite get the API change. I thought all we would need to do is put a try/catch around the validator code in the web interface (code from gh-pages)
• In terms of limiting the number displayed errors in web interface - totally for it! I remember we talked about this in person - if it's not yet implemented could you open an issue about it?

from bids-validator.

No worries.

I like the idea of turning everything into a sync, it will not only allow us to do simple try catch, but also make it easier for non node.js savvy people to contribute to the project

Sounds good. I'll start converting what parts can be synchronous as I'm making other updates.

I don't quite get the API change. I thought all we would need to do is put a try/catch around the validator code in the web interface (code from gh-pages)

The try/catch strategy won't work as expected when wrapping an async function. The try/catch block will end as soon as the async function starts and won't catch anything that happens while it's executing. We can make large portions of the validator code synchronous. This will simplify unexpected error handling in those sections, but it's important to keep other aspects of the validator asynchronous (like file reading) mostly because these operations are more resource/time intensive and if they're synchronous they will "block" and the browser UI will be totally frozen until these operations finish.

I'm not sure if the API change is totally necessary at this time. The idea though is to provide a way to pass off errors that are errors with the code execution and not errors with the dataset. This would allow the validator consumer (web-app or server-app) to act accordingly when the validator fails. I don't know of an issues that fit this need right now but if we encounter errors that are not specific to the domain of BIDS we could use this strategy.

An example might be the browser/node-version being used doesn't support some language feature we're using and we could return an appropriate error but in a different way than the validator returns errors about a dataset. It would also provide a way to funnel out issues caught in any synchronous try/catch blocks that we may add (as mentioned above).

In terms of limiting the number displayed errors in web interface - totally for it! I remember we talked about this in person - if it's not yet implemented could you open an issue about it?

Yes we did talk about it in person. I'll open an issue and probably tackle this one very soon.

from bids-validator.