timers: force timeout to signed int in enroll()

[cjihrig]

Closes #8618

[Raynos]

This is a really important fix.

The fact that setTimeout on a socket casts a string to no timeout is a really nasty production bug.

[misterdjules]

@cjihrig I agree that this change fixes #8618, but it still makes Socket.prototype.setTimeout do nothing silently if msecs is a value that can't be converted to a number. Shouldn't we throw an error when the msecs argument of Socket.prototype.setTimeout is not a number?

Same question for timers.enroll.

[Raynos]

@misterdjules 👍 for throwing for non-numbers. thats an even better idea.

[cjihrig]

@misterdjules updated to throw on non numeric input in both functions. timers.enroll() is only called from two places (one is Socket.prototype.setTimeout(), and the other where the delay is guaranteed to be a number), so repeating the check in there might be overkill, but I think it's more important to check in that function. Throwing also makes testing much simpler.

EDIT: I think it might be good to leave the case to a signed int in place (I removed it in the latest commit). But I think it would help with NaN delay values.

[misterdjules]

After giving it more thought, it seems that this test would throw on all invalid use cases:

if (!util.isNumber(msecs) || !isFinite(msecs) || msecs < 0)
  throw new TypeError('msecs must be a non-negative finite number');

Using util.isNumber makes it more consistent with the rest of the code base.

What do you think?

[orangemocha]

+1 for using util.isNumber
+1 for throwing on negative and not finite values, although I think RangeError would be more appropriate in that case (so it would need two separate checks).

[misterdjules]

@cjihrig Thank you very much for taking the time to update this PR. I think it's getting better.

I think that NaN values should also make the code throw the same error, so I don't think we would need to put back the conversion to a signed int. I added a few comments inline, please let me know what you think.

Also thank you @Raynos for your feedback. it is very much appreciated!

[cjihrig]

@trevnorris @tjfontaine can you weigh in here? @misterdjules and I were discussing the best way to handle edge cases. Specifically, timeouts < 0, Infinity, and NaN. Current behavior is to silently ignore. Should we begin throwing on these?

[trevnorris]

@cjihrig I recommend doing a simple range check and uint coercion. For example:

function n(fn, timeout) {
  if (timeout < 0) timeout = 0;
  timeout = timeout >>> 0;
...

EDIT: To elaborate, I don't believe throwing is a good idea. For kicks, I may want to pass a numeric string like '0xdead'. The uint coercion above will take care of this.

[cjihrig]

@misterdjules does this work for you? If so, I'll update the code.

[misterdjules]

@trevnorris What don't you like about throwing an error in this case?

I have the same opinion as before. I agree that coercing msecs to an unsigned integer is a small improvement over what we have currently, however this API would still be flaky to me, because it would fail silently sometimes.

In this specific case, if someone passes a string that cannot be converted to a number to Socket.prototype.setTimeout, the timer will never time out. It is difficult to investigate because nothing happens. If an error is thrown immediately, it becomes much easier to know the cause of the problem.

Do we have general guidelines on how our APIs should handle erroneous programmer input? I like the idea of failing as early as possible. In my opinion, it makes troubleshooting issues way easier.

timers.setTimeout and timers.setInterval coerce the delay parameter to a number, but I don't think we have to preserve that behavior for Socket.prototype.setTimeout.

We could make this PR just coerce msecs to an unsigned integer. It would improve the current API slightly and fix some of its issues. However I think this could be a good opportunity to make it actually robust.

If we're concerned with backward compatibility, then maybe we could land a fix that only coerces the delay in v0.10, and another one that throws errors for any erroneous programmer input in v0.12 or later?

[misterdjules]

I don’t want to stop anyone from making progress though, I just want to make sure that we explore the opportunity of making this API more robust. I would be fine with exploring that in an unstable branch.

[misterdjules]

@tjfontaine @chrisdickinson @orangemocha @indutny Your input would be appreciated! :)

[trevnorris]

@misterdjules

In this specific case, if someone passes a string that cannot be converted to a number to Socket.prototype.setTimeout

I did not consider that the timeout is removed in the case where the timeout == 0. I still believe numeric strings should be passable. That can be checked with if (timeout >>> 0 == timeout) (the == does a string coercion). So I'll change my snippet to the following:

Socket.prototype.setTimeout = function(msecs, callback) {
  if (msecs >>> 0 != msecs)
    throw new TypeError('msecs should be uint32');
  msecs = msecs >>> 0;

That will handle everything that doesn't convert properly (NaN, Infinity, negative numbers, etc.).

[tjfontaine]

I agree, lets take this time to make this interface crisp. The API clearly only takes a positive number, invalid type inputs should generate TypeError, and values that are outside of the valid range should throw RangeError. I think we should also update the documentation to reflect this.

For what it's worth coercion in our APIs is often the root of subtle unfortunate bugs, in this particular case it's very difficult to believe passing a string to the API was what they actually meant to do (even if that string would parse to a valid number).

But the good news is it's always easier to relax that constraint later than it is to make it more restrictive.

[orangemocha]

+1 for throwing on invalid input in general. I would suggest throwing RangeError for non-positive or non-finite numbers (but TypeError for non numeric arguments).

[cjihrig]

Closing as this is against master. Will open a new PR against 0.12 based on feedback and reference this PR. Thanks!