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

End-of-message padding seems wrong at least on Mac OS X #16

Closed manuelsantillan closed 10 years ago

manuelsantillan commented 10 years ago

Hi Alberto,

We're experiencing some intermittent issues when sending large strings to Rserve: connection is closed unexpectedly before getting a response depending on the data length.

Experimenting a little bit, I've found that the 2-byte padding you add at the end of the buffer with charCode(1) (at rio.js#sendCommand, when you add byte 0x1 when n & 3!=0) seems to be related.

I've experimented with n & 15 and n & 63 (and recalculating the length of course), and it seems to fix it.

Do you know if Rserve is expecting 2-byte words or bigger words? If Rserve expects 4-byte words or 8-byte words, then my fix does make a lot of sense. If not, then it is very weird...

Cheers!

albertosantini commented 10 years ago

Hey, thanks for the feedback.

I ported that code from the simple php client (src/client/php/simple.php) contained in Rserve (1.7-3) distribution.

function mkp_str($cmd, $string) {
    $n = strlen($string) + 1; $string .= chr(0);
    while (($n & 3) != 0) { $string .= chr(1); $n++; }
    return mkint32($cmd) . mkint32($n + 4) . mkint32(0) . mkint32(0) . chr(4) . mkint24($n) . $string;
}

Maybe it is wrong.

The string should pad to 4-byte boundaries: ((cmd.length + 1) + 3) & ~3;

Please, any time to contribute the fix and the test?

manuelsantillan commented 10 years ago

Created the PR with the fixture and the patch.

albertosantini commented 10 years ago

Merged.

I prepare a new release.

albertosantini commented 10 years ago

Published 1.3.1.

Gracias, Manuel.

Anyway we need to find a better way to test it, because the fixture was too large.