/cc @jasnell @targos @thefourtheye

LGTM, but should the test be in a separate commit?

LGTM, but should the test be in a separate commit?

I'm really not sure, I can check previous commits. To me it makes sense to include the test with the commit removing the regression.

The spec documents that the second argument must be an integer.
Infinity is being interpreted as 0... instead of MAX_INT
(perhaps this should be a fix upstream to v8).

Sadly, it is as per the spec only.

String.prototype.split says,

If limit is undefined, let lim = 232–1; else let lim = ToUint32(limit).

and 9.6 ToUint32: (Unsigned 32 Bit Integer) says,

If number is NaN, +0, −0, +∞, or −∞, return +0.

@thefourtheye great idea, I've restructured this commit

@evanlucas I'll need another LGTM. Rather than reverting I've added an extra check for infinity. It now makes much more sense to include the test

@nodejs/testing is there a preference to writing more smaller tests, or putting this in with another test already testing querystring?

Yea, I like this. LGTM. Definitely need CI on this one though to make sure arm is happy with it too

Adding a new test is fine in this case, and it would be better to have this issue also mentioned somewhere in the comments.

CI: https://ci.nodejs.org/job/node-test-pull-request/1534/

I couldn't think of any other corner cases. LGTM.

It might be a little more efficient to add the isFinite() check to the line if (options && typeof options.maxKeys === 'number') {. Then you would only get the overhead if a number is provided as input.

LGTM with a comment.

In that case we can simply do if (options && Number.isFinite(options.maxKeys)

@nodejs/testing is there a preference to writing more smaller tests, or putting this in with another test already testing querystring?

Speaking for myself only, I definitely prefer smaller, isolated tests.

/cc @manvalls

@thealphanerd I think this is ok, lgtm

@Trott:

@nodejs/testing is there a preference to writing more smaller tests, or putting this in with another test already testing querystring?

Speaking for myself only, I definitely prefer smaller, isolated tests.

One thing to keep in mind is that separate tests take longer to run in CI than compacting them into fewer tests. Primarily due to very slow startup time on slower ARM machines but also it seems like this AIX and musl libc based systems have slow exit times (ref #5056).

More files, separate tests is certainly the nicer way to approach it but I just wanted to register the above concern so that @nodejs/testing is aware of this and can (maybe) take it into account when forming opinions on how to best structure tests.

@cjihrig @thefourtheye if we made the suggested change then maxKeys will always = 1000, this would also result in a behaviour change

@rvagg I've updated the test with a comment that includes the github link, with sha and line numbers.

As you can expect this link is far over the max characters per line. I've split it on to two lines, hopefully that isn't confusing

@thealphanerd no, this is what I was proposing. It should be functionally equivalent to what you have, but slightly more efficient. And, as @thefourtheye pointed out, the last two conditions can be condensed to Number.isFinite().

  var maxKeys = 1000;
  if (options && typeof options.maxKeys === 'number' && isFinite(options.maxKeys)) {
    maxKeys = options.maxKeys;
  }

  // maxKeys <= 0 means that we should not limit keys count
  if (maxKeys > 0) {

maxKeys is set just above though. Meaning that if(maxKeys > 0) will be true if options.MaxKeys === Infinity

thus the split will be called with maxKeys set to 1000 which is still a behavior change.

To double check that I am correct I have implemented your change and run it against the tests and we get a failure

AssertionError: 1000 == 10000

Sorry, you're right. I was thinking we wanted maxKeys to be the default value on bad input. The existing behavior seems silly... Use 1000 as a default, but if a number is provided use that, but if it's not positive and finite, ignore it completely.

Use 1000 as a default, but if a number is provided use that, but if it's not positive and finite, ignore it completely.

Ya, that doesn't look right. Can we fix this behavior here?

@thefourtheye I think we can fix that behavior, but that would be a Major change. In the mean time we do need to patch back the behavior that is expected (imho).

Or are you arguing that the behavior itself was a bug?

@thealphanerd Though it seems odd, as you pointed out, our priority here is to get this fixed asap, with the current functionality. Perhaps we can improve this in a separate patch. LGTM.

I think it is also worth mentioning that #5012 rewrite all the code this is touching.

it seems like this AIX and musl libc based systems have slow exit times (ref #5056).

Technically, exit itself is quick. What happens is that the parent exits without reaping the child processes, so init (pid 1) has to reap those zombies. The slow timeout on Alpine Linux was due to busybox init has a "design flaw" that will intentionally delay the reaping of the orphaned child processes. This has actually nothing to do with musl libc and I am pretty sure that busybox init with glibc has the exact same issue. Most likely this is what AIX init does too.

Exit times are fast. But someone/something needs to reap the dead processes. Ideally should the parent process take care of that instead of letting init (pid 1) do it.

Rubber-stamp LGTM

Landed as 878bcd4

Express must be happy now, right?

Thanks @ncopa, that's very interesting background