albertosantini / node-rio

Integration with Rserve, a TCP/IP server for R framework
https://github.com/albertosantini/node-conpa
MIT License
176 stars 35 forks source link

Possible fix for #13. Includes error strings/network errors propagation ... #14

Closed manuelsantillan closed 10 years ago

manuelsantillan commented 10 years ago

...and a control for callbacking just once. Alberto, take a look at this proposal. Not really tested it yet, but I think the idea is clear: it propagates error strings in callbacks and network errors as well, instead of an error flag. Additionally, callback is wrapped in a closure that ensures it only gets called once.

What do you think about it?

Important: I've not made any tests yet, this pull request is only to show the pattern. In case you wanna merge, we should test it before.

manuelsantillan commented 10 years ago

BTW, Alberto, could you point me to a link or source files where error codes are listed?

albertosantini commented 10 years ago

Usually I give a look at Rserve source code for the error codes. https://github.com/s-u/Rserve/blob/master/src/Rsrv.h

I begin to test the code, but it seems there is something hurting. Investigating...

manuelsantillan commented 10 years ago

Yep i'm looking at those tío. Seems that asserts are expecting err==true instead of just checking for err existence

El sábado, 26 de abril de 2014, albertosantini notifications@github.com escribió:

Usually I give a look at Rserve source code for the error codes. https://github.com/s-u/Rserve/blob/master/src/Rsrv.h

I begin to test the code, but it seems there is something hurting. Investigating...

— Reply to this email directly or view it on GitHubhttps://github.com/albertosantini/node-rio/pull/14#issuecomment-41463425 .

Manuel Santillán Socio - Director de Operaciones www.solvver.com

albertosantini commented 10 years ago

I gave a look at vows issues and it seems a problem in vows. I think I will switch to mocha.

manuelsantillan commented 10 years ago

I think there's no need to switch. If I'm right, your problem is due to vows not letting errors propagate for assertion -> https://github.com/flatiron/vows/issues/183

I've changed the grunt file to change vows error handling. Take a look and see if that's fine for you

manuelsantillan commented 10 years ago

BTW, my PR builds a literal for the exception. Might as well return a full-blown error object with error code and error message for better upwards handling. For my use case it's fine with string messages, as I only need to have human-readable error objects to see what's going on in production without debugging...

albertosantini commented 10 years ago

Nice.

I tried suite.options.error = false; and it works too.

I prefer to implement the callback hack in the connect method.

function connect(netOpts, callback, isRecordMode) {
    var client = null,
        recordStream,
        isCallbacked = false,
        cb = function (err) {
            if (!isCallbacked) {
                callback(err);
                isCallbacked = true;
            }
        };
...

I agree with your approach about using a string message vs. an error object. Maybe for the version 2.x I will split rio.js and implement a better error handling.

Tomorrow I give a look to merge the PR.