Fix BSER for nodejs #393

danez · 2016-11-30T22:51:12Z

This fixes several issues with the nodejs implementation of BSER. Javascript is tricky as it treats all numbers as float, so there are some drawbacks of this implementation.

Fixed the MAX_INT16 and MAX_INT32 constants to be correct. They were both +1 which led to argument is out of bounds if trying to dump 32768 or 2147483648 as int
Fixed detection of integers inside dump_any() by checking if it is an integer with the polyfill outline in this MDN page.
- This has the drawback that a float cannot be enforced anymore, as 1.0 will also be detected as int because 1.0 === 1
- The only real solution to properly fix all cases which I can see, is to have a new Double() class or new Integer()class, which a user would need to supply to enforce the type.
Changed readInt() when reading an 64bit integer to check if the value can be handled by node.js. If yes it returns the primitive number otherwise returns the instance of Int64. This imho is nice, even if the return value in some cases could be either number or Int64 (not sure what is the actual limit)
- This is a breaking change for users who always expect a number > MAX_INT32 to be an instance of Int64. (e.g. mtime_ms in watchman was always a Int64 and could now be a number)
- If not desired, let me know, I can revert this.
Changed the tests to use deepStrictEqual to ensure primitive types are checked with ===

Fixes #392

This fixes the MAX_INT16 and MAX_INT32 constant to be correct and makes numbers without fraction (1 or 1.0) to be encoded as ints Also changes the return values when reading an int64 to be a primitive number if possible. This is a breaking change.

wez · 2016-11-30T23:04:30Z

Wow, this is little embarrassing... thanks for taking this on!

I think I'm fine with the idea of this making a breaking API change so long as it doesn't bite anyone in practice. Floats a relatively rare in the FB usage of watchman and BSER, so I don't anticipate that that will hurt any of the commonly used tools here.

cc: @mostafaeweda for Nuclide and js-fu on the FB side. I wonder if this might explain some of that weird BSER errors you mentioned a while back? Do you anticipate that this will break Nuclide? Would you mind also reviewing the JS here; clearly my implementation was off in some important details :-p

wez · 2016-11-30T23:05:36Z

(CI may be a bit flaky at the moment because of some pretty major changes in the core that haven't been checked out on all platforms)

mostafaeweda · 2016-12-01T01:27:36Z

node/bser/index.js

@@ -120,6 +120,9 @@ Accumulator.prototype.peekInt = function(size) {

 Accumulator.prototype.readInt = function(bytes) {
  var ival = this.peekInt(bytes);
+  if (ival instanceof Int64 && isFinite(ival.valueOf())) {


when is is expected to not be finite?
In that case, the return value here can either be number or Int64
I'd rather check the isFinite on the consumer side of this function's return value.

when we decode bser and it contains an 64bit integer, we always create an instance of Int64 for it in peekInt(). For up to 2^53 − 1 javascript engines could display and handle numbers just fine, because the engines use floating point representations internally. For these numbers valueOf() will return the number, for everything above and up to MAX_INT64 ( 2^63-1) it will return Infinity.

So what we do here is simply checking if it is still a number that the javascript-engine can handle and if yes use the number instead of the Int64 Instance. readInt returns anyway either number or Int64 even without my change, depending if the integer is 8/16/32bit (number) or 64bit (Int64)

Although especially in javascript it does not make sense to return Int64 instances to consumers of bser imho. Because either I try to convert it to number anyway when I receive the Int64 instance because I want to use it for calculations or if I can't convert it because it is to high and javascript cannot handle it natively the only thing I can do with Int64 is forward it to watchman/bser itself again or use it as string, as calculations cannot be done with Int64 instances.

But yes up to you.

mostafaeweda · 2016-12-01T01:27:52Z

node/bser/index.js

@@ -120,6 +120,9 @@ Accumulator.prototype.peekInt = function(size) {

 Accumulator.prototype.readInt = function(bytes) {
  var ival = this.peekInt(bytes);
+  if (ival instanceof Int64 && isFinite(ival.valueOf())) {
+    ival = ival.valueOf();
+  }
  this.readOffset += bytes;
  return ival;


let's have a consistent return result - either number or Int64.

As I wrote in the other comment, it already returns both depending on the size of the integer.

mostafaeweda · 2016-12-01T01:29:08Z

Nuclide sits at the top level, far away from bser - https://github.com/facebook/nuclide/blob/master/pkg/nuclide-watchman-helpers/lib/WatchmanClient.js#L284-L286 expecting request / response objects
So, I highly doubt this would break it, but it may indeed fix the bser deserialization errors we had every now and then.

Initially we use the timestamp that webpack provides to tell watchman to watch since this time. Afterwards we always keep the last clock value of subscription result, which is more accurate. node-int64 is a dependency now due to bug facebook/watchman#393 Fixes #11

facebook-github-bot · 2016-12-01T22:06:58Z

@wez has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

* Handle changes which happen during compilation Initially we use the timestamp that webpack provides to tell watchman to watch since this time. Afterwards we always keep the last clock value of subscription result, which is more accurate. node-int64 is a dependency now due to bug facebook/watchman#393 Fixes #11 * Add test * rename test * Optimize tests, mark some lines as covered * stabilize tests * lower timeout

danez · 2016-12-14T12:45:28Z

Thanks, would be nice if you could also make a new release. :)

wez · 2016-12-15T08:35:47Z

Yeah, we're just getting some things squared away; also need to release the python bindings shortly as well

Fix BSER for nodejs

0ca5800

This fixes the MAX_INT16 and MAX_INT32 constant to be correct and makes numbers without fraction (1 or 1.0) to be encoded as ints Also changes the return values when reading an int64 to be a primitive number if possible. This is a breaking change.

facebook-github-bot added GH Review: review-needed CLA Signed labels Nov 30, 2016

facebook-github-bot added the CLA Signed label Nov 30, 2016

mostafaeweda reviewed Dec 1, 2016

View reviewed changes

danez mentioned this pull request Dec 1, 2016

Handle changes which happen during compilation researchgate/webpack-watchman-plugin#78

Merged

facebook-github-bot added the Import Started label Dec 1, 2016

facebook-github-bot closed this in 75645d1 Dec 2, 2016

danez deleted the fix-bser branch December 14, 2016 12:44

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix BSER for nodejs #393

Fix BSER for nodejs #393

danez commented Nov 30, 2016 •

edited

Loading

wez commented Nov 30, 2016

wez commented Nov 30, 2016

mostafaeweda Dec 1, 2016

danez Dec 1, 2016 •

edited

Loading

mostafaeweda Dec 1, 2016

danez Dec 1, 2016

mostafaeweda commented Dec 1, 2016

facebook-github-bot commented Dec 1, 2016

danez commented Dec 14, 2016

wez commented Dec 15, 2016

Fix BSER for nodejs #393

Fix BSER for nodejs #393

Conversation

danez commented Nov 30, 2016 • edited Loading

wez commented Nov 30, 2016

wez commented Nov 30, 2016

mostafaeweda Dec 1, 2016

Choose a reason for hiding this comment

danez Dec 1, 2016 • edited Loading

Choose a reason for hiding this comment

mostafaeweda Dec 1, 2016

Choose a reason for hiding this comment

danez Dec 1, 2016

Choose a reason for hiding this comment

mostafaeweda commented Dec 1, 2016

facebook-github-bot commented Dec 1, 2016

danez commented Dec 14, 2016

wez commented Dec 15, 2016

danez commented Nov 30, 2016 •

edited

Loading

danez Dec 1, 2016 •

edited

Loading