support asynchronous test cases properly #4529

alexlamsl · 2021-01-09T19:04:06Z

@kzc you may find this interesting 😉

Caveats:

when async is in use, anything relying on Unicode will break (at least on Windows)
it is very slow 🐢

kzc · 2021-01-09T19:33:59Z

test/sandbox.js

    } while (prev !== stdout);
    return stdout;
-} : run_code;
+} : semver.satisfies(process.version, "<8") ? run_code_vm : function(code, toplevel, timeout) {
+    return (/async function/.test(code) ? run_code_exec : run_code_vm)(code, toplevel, timeout);


I like the fact that it differentiates between async test cases and regular ones for perf reasons, but that regex won't catch async arrow functions or when the tokens span multiple lines.

Yeah I forgot to append [WIP] to this PR − not planning to merge this, just thought I'd clean up the stage a bit so you can have a go when you want to do so 😉

kzc · 2021-01-09T19:39:40Z

Very cool.

Can we force certain tests to be async even without an async or await keyword? For example - a classic ES5 test invoking setTimeout? The test could have a property async: true (or perhaps spawn: true) for such tests to force the use of run_code_exec.

kzc · 2021-01-09T19:45:12Z

when async is in use, anything relying on Unicode will break (at least on Windows)

That's very odd. Must be a Windows thing. Can you force a certain Windows unicode code page or something like that?

it is very slow 🐢

No surprise there - but at least it works - that's the important thing.

It should be possible to squeeze some extra performance from it with a reusable child process and a wire protocol from the main program to avoid the process spawning overhead.

alexlamsl · 2021-01-09T20:26:16Z

Can you force a certain Windows unicode code page or something like that?

I tried to no avail − it's a Node.js bug, as I can see and print Unicode in Command Prompt without issues.

Node.js v15 also screws up the ANSI colour codes, so no surprises there. I have tried to make Node.js use Buffer instead of strings for stdio, but that doesn't seem to make a difference.

alexlamsl · 2021-01-09T20:30:43Z

It should be possible to squeeze some extra performance from it with a reusable child process and a wire protocol from the main program to avoid the process spawning overhead.

Since some test cases creates and modifies global variables, will need to make sure everything's cleaned up properly.

And then there's the problem of using any asynchronous API calls which requires extensive rewrites of all the users of sandbox.run_code() − that I'm not thrilled about in the slightest...

alexlamsl · 2021-01-09T20:33:04Z

Can we force certain tests to be async even without an async or await keyword?

Could do − as I said earlier, this PR is more of a proof of concept at this stage, as there will be kinks to work out in order for it to work across all Node.js versions and OS platforms.

alexlamsl · 2021-01-09T20:38:17Z

BTW child_process.execSync() is only available in Node.js v0.12+, so we can't really test on a ES5 runtime for things like setTimeout() − but then I guess that's not a major issue.

kzc · 2021-01-09T21:13:32Z

Great proof of concept in any event.

I don't recall whether function*/yield test cases were also limited by the vm sandbox relying on next tick.

Node.js v15 also screws up the ANSI colour codes

I hate that new useless color feature in NodeJS. Should just disable it with that env var NO_COLOR and call it a day.

alexlamsl · 2021-01-09T21:50:59Z

Right, let's see if my workaround for the newly discovered quirk on Node.js v0.12 works.

I've expanded the regex to cover all forms of async and the timer functions as well, so I guess we don't need a special flag for that.

I don't recall whether function*/yield test cases were also limited by the vm sandbox relying on next tick

Anything that gets scheduled on a queue would escape vm sandbox 😓

I hate that new useless color feature in NodeJS.

At least you only hate that one feature... 😉

kzc · 2021-01-09T21:57:31Z

I've expanded the regex to cover all forms of async and the timer functions as well, so I guess we don't need a special flag for that.

The dozen non-asynchronous tests using async variable and function names would be slowed down by exec needlessly. Sometimes would be useful to override behavior.

alexlamsl · 2021-01-09T22:52:27Z

Fair point − I was more concerned with ufuzz not being able to use such a feature flag, but yes if we do have a sizable volume of synchronous compress tests that run via execSync the slowdown would be noticable.

alexlamsl · 2021-01-09T22:54:33Z

Looks like what happened to Node.js v0.12 back in #4495 is happening to Node.js v6 as well 👻

alexlamsl · 2021-01-10T16:40:07Z

test.js

if (0) (function() {
/*
 * abcdefghijklmnopqrstuvwxyz
 * abcdefghijklmnopqrstuvwxyz
 * abcdefghijklmnopqrstuvwxyz
 * abcdefghijklmnopqrstuvwxyz
 * abcdefghijklmnopqrstuvwxyz
 * abcdefghijklmnopqrstuvwxyz
 * abcdefghijklmnopqrstuvwxyz
 * abcdefghijklmnopqrstuvwxyz
 * abcdefghijklmnopqrstuvwxyz
 * abcdefghijklmnopqrstuvwxyz
 * abcdefghijklmnopqrstuvwxyz
 * abcdefghijklmnopqrstuvwxyz
 * abcdefghijklmnopqrstuvwxyz
 * abcdefghijklmnopqrstuvwxyz
 * abcdefghijklmnopqrstuvwxyz
 * abcdefghijklmnopqrstuvwxyz
 * abcdefghijklmnopqrstuvwxyz
 * abcdefghijklmnopqrstuvwxyz
 * abcdefghijklmnopqrstuvwxyz
 * abcdefghijklmnopqrstuvwxyz
 * abcdefghijklmnopqrstuvwxyz
 * abcdefghijklmnopqrstuvwxyz
 * abcdefghijklmnopqrstuvwxyz
 * abcdefghijklmnopqrstuvwxyz
 * abcdefghijklmnopqrstuvwxyz
 * abcdefghijklmnopqrstuvwxyz
 * abcdefghijklmnopqrstuvwxyz
 * abcdefghijklmnopqrstuvwxyz
 * abcdefghijklmnopqrstuvwxyz
 * abcdefghijklmnopqrstuvwxyz
 * abcdefghijklmnopqrstuvwxyz
 */
})();
(function({
    [function() {
        console.log(typeof f);
    }()]: a,
}) {})(0);

$ node -v
v6.17.1
$ cat test.js | node

$ node test.js
undefined

$ node -v
v8.17.0
$ cat test.js | node
undefined

$ node test.js
undefined

kzc · 2021-01-10T16:52:37Z

Buffer overflow in Node v6?

alexlamsl · 2021-01-10T17:24:54Z

Given the fact that it relies on the destructuring function parameter with computed key to trigger, I'd lean towards some parser bug on the "slow" path, routed by input byte size, which isn't present on the "fast" path.

kzc · 2021-01-11T00:30:38Z

exec won't be fast enough for ufuzz and test/reduce without some form of child process reuse - perhaps even a pool of child processes. Might this dual vm/exec sandbox be used for test/compress only for the time being, with ufuzz and test/reduce only using vm?

alexlamsl · 2021-01-11T00:43:55Z

Any idea how to make a pool of child processes and perform synchronous send/receive on Node.js? 😅

I'm in no hurry to merge this PR − actually amused that all the tests passed so soon, but will just keep it tracking master for now, get more ufuzz exposure in parallel to shake out other potential compatibility issues.

kzc · 2021-01-11T02:21:21Z

Any idea how to make a pool of child processes and perform synchronous send/receive on Node.js?

I guess it wouldn't be. Would have to shim Promise support into the compress test runner.

alexlamsl · 2021-01-11T16:01:51Z

I guess it wouldn't be. Would have to shim Promise support into the compress test runner.

I've found a performant solution reusing a child process without turning sandbox.run_code() asynchronous − but that reveals the next roadblock:

Without the luxury of child process exiting to signal completion, how do you know within the child process that all of asynchronous test case has finished?

alexlamsl · 2021-01-16T18:36:15Z

An order of magnitude slower than typical test/reduce vm cases?

That's the best case − for some reason the variation between runs is huge as well.

The sandbox ought to have an option to force the use of exec (or vm for that matter), and in turn it could be enabled by the CLI with --test-reduce=exec or --test-reduce=vm.

Could do, but then these failures are so esoteric, and strictly applies to a code base which showed little intention of tackling them, I'd prefer the existing behaviour of watching v8 die than getting bugged by another false positive TBH.

alexlamsl · 2021-01-16T18:40:38Z

The (currently discovered) showstopper for using later version of Node.js for ufuzz is the bypassing of timeout during vm.runInThisContext() which effectively halts the job until it is killed.

run_code_exec() would have workaround that, however I don't see a way to effectively detect these cases, and the frequency of occurrence would take an unacceptable toil on the fuzzing rate as well.

kzc · 2021-01-16T19:42:17Z

these failures are so esoteric, and strictly applies to a code base which showed little intention of tackling them, I'd prefer the existing behaviour of watching v8 die than getting bugged by another false positive TBH.

That's fair, I guess it's strictly a V8 issue, not something that can be remedied within uglify minify.

The (currently discovered) showstopper for using later version of Node.js for ufuzz is the bypassing of timeout during vm.runInThisContext() which effectively halts the job until it is killed.

vm is so limited in NodeJS, and its utility is decreasing with each new release. Makes you wonder what use case it was designed for.

kzc · 2021-01-16T19:53:52Z

That's the best case − for some reason the variation between runs is huge as well.

In this PR, is run_code_exec presently used more than it should due to ufuzz generating synchronous programs with variables and functions named async? i.e., what is the rough percentage of ufuzz generated programs with an async keyword or symbol name?

alexlamsl · 2021-01-16T20:56:54Z

vm is so limited in NodeJS, and its utility is decreasing with each new release. Makes you wonder what use case it was designed for.

The kindest thing I can say is "hindsight's 20/20" 😏

In this PR, is run_code_exec presently used more than it should due to ufuzz generating synchronous programs with variables and functions named async?

Oh that issue is mostly resolved now − ufuzz timings for this PR is now within acceptable limits. There are occasional jobs getting stuck for some mysterious reasons, but they are both rare and shouldn't stay stuck for the scheduled jobs as they are determined and killed by the parent process instead.

My plan is to merge this after the next release − it's just that I've been getting a trickling feed of ufuzz failures on master which is why this is still hanging around for further studies.

Assuming an uneventful evening I'll make that release tomorrow and we can turn our full attention to watching this PR does wonders 😎

kzc · 2021-01-20T04:50:51Z

Is fuzzing slower now with the hybrid vm/exec sandbox?

alexlamsl · 2021-01-20T07:38:47Z

Nothing measurable beyond the usual variations between runs yet.

Not helped by the fact that GitHub seems to be having issues with scheduling workflows, or marking one as "Completed" regardless of whether its jobs are running or even started.

Whatever they thought to be "resolved" clearly hasn't 🤔
https://www.githubstatus.com/incidents/rk5rhrklkdk3

alexlamsl · 2021-01-20T11:18:50Z

Let's hope these don't pop up too frequently 👻
https://github.com/mishoo/UglifyJS/runs/1732658088?check_suite_focus=true#step:4:1603

kzc · 2021-01-20T16:33:21Z

I'm trying to grok the transform that avoids a core dump in NodeJS v8.x...

$ echo '(async (foo = [ ([] = 0), ...0 ]) => 0)();' | uglify-js -bc
(async (foo = ([] = 0, [ ...0 ])) => 0)();

Is the default value [ ([] = 0), ...0 ] equivalent to ([] = 0, [ ...0 ]) ?

alexlamsl · 2021-01-20T17:28:51Z

No, they are not equivalent − but that's alright because foo is unused:

UglifyJS/lib/compress.js

Lines 6206 to 6210 in 7793c6c

    
           function trim_default(trimmer, node) { 
        
               node.value = node.value.transform(tt); 
        
               var name = node.name.transform(trimmer); 
        
               if (!name) { 
        
                   var value = node.value.drop_side_effect_free(compressor);

$ echo '(async (foo = [ ([] = 0), ...0 ]) => 0)();' | uglify-js -bc unused=0
(async (foo = [ [] = 0, ...0 ]) => 0)();

kzc · 2021-01-20T17:46:29Z

that's alright because foo is unused

Ah, yes of course. The side effects are retained. Thanks.

Side note... test/reduce could drop the async designation of the function and still produce a reduced test case with a core dump in NodeJS v8.x. But it's trickier with concrete classes for all the various Async function combinations instead of an async property - a new non-async function node would have to be created and the attributes copied.

alexlamsl · 2021-01-20T21:13:54Z

Indeed − taking the async off the arrow function and using run_code_exec() unconditionally yields:

((foo = [ [] = 0, ...0 ]) => 0)();

... which works on Node.js 6 👻

alexlamsl · 2021-01-21T01:25:36Z

This is the same v8 (non-crashing) bug, but this time uglify-js triggers rather than suppresses it:

// reduced test case (output will differ)

// (beautified)
(async (NaN_2 = 0(...[ ...0, "object" ])) => {})();
// output: 
// minify: SyntaxError: Rest parameter must be last formal parameter
// options: {
//   "mangle": false,
//   "output": {
//     "v8": true
//   },
//   "validate": true
// }

kzc · 2021-01-21T04:11:17Z

You can't win 'em all.

kzc · 2021-01-26T07:55:21Z

when async is in use, anything relying on Unicode will break (at least on Windows)

Can you force a certain Windows unicode code page or something like that?

I tried to no avail − it's a Node.js bug, as I can see and print Unicode in Command Prompt without issues.

Not sure if you're still experiencing this issue. If so, here's a windows unicode bug workaround:

evanw/esbuild@93423e5

alexlamsl · 2021-01-26T12:06:55Z

Indeed − thanks for the pointer 👍

Node's fs.write API is broken when writing Unicode to stdout and stderr on Windows, and this will never be fixed:

... I see we share the same enthusiasm 😏

alexlamsl · 2021-01-26T12:43:57Z

Unfortunately we are running into a different Unicode bug than their workaround has provided (and no it's not because Command Prompt doesn't support Unicode by default − the only program that has b0rk3n output is Node.js)

alexlamsl · 2021-01-26T12:53:45Z

Specifically, it's the unpaired surrogate characters which are being mistranslated.

kzc · 2021-01-26T15:35:01Z

The unicode code page hack chcp 65001 mentioned in that NodeJS issue doesn't resolve it either?

alexlamsl · 2021-01-26T15:39:54Z

The unicode code page hack chcp 65001 mentioned in that NodeJS issue doesn't resolve it either?

First thing I tried after Google-fu − does not change any behaviour on later versions of Windows 😏

kzc reviewed Jan 9, 2021

View reviewed changes

alexlamsl changed the title ~~support async test cases properly~~ [WIP] support async test cases properly Jan 9, 2021

alexlamsl force-pushed the sandbox branch 2 times, most recently from b1da9db to 96ef385 Compare January 9, 2021 21:50

alexlamsl force-pushed the sandbox branch from 96ef385 to 77a2f0f Compare January 10, 2021 03:36

alexlamsl force-pushed the sandbox branch from 77a2f0f to 1d3b32c Compare January 10, 2021 17:21

alexlamsl force-pushed the sandbox branch 3 times, most recently from 3b3b6c3 to 61c3e4b Compare January 10, 2021 22:31

alexlamsl force-pushed the sandbox branch from 61c3e4b to b48bc06 Compare January 11, 2021 01:14

alexlamsl force-pushed the sandbox branch from b48bc06 to 6d33edc Compare January 11, 2021 14:29

alexlamsl force-pushed the sandbox branch from 6d33edc to 5aa0571 Compare January 11, 2021 19:39

alexlamsl force-pushed the sandbox branch 4 times, most recently from 82e13a7 to fbfd852 Compare January 19, 2021 02:42

support asynchronous test cases properly

273c7bc

alexlamsl force-pushed the sandbox branch from fbfd852 to 273c7bc Compare January 19, 2021 19:52

alexlamsl merged commit d37ee4d into mishoo:master Jan 19, 2021

alexlamsl deleted the sandbox branch January 19, 2021 23:27

alexlamsl mentioned this pull request Jan 26, 2021

workaround tty bugs on Node.js #4594

Merged

alexlamsl mentioned this pull request Jan 26, 2021

fix corner case in inline #4596

Merged

support asynchronous test cases properly #4529

support asynchronous test cases properly #4529

Conversation

alexlamsl commented Jan 9, 2021

kzc Jan 9, 2021 • edited Loading

Choose a reason for hiding this comment

alexlamsl Jan 9, 2021

Choose a reason for hiding this comment

kzc commented Jan 9, 2021 • edited Loading

kzc commented Jan 9, 2021

alexlamsl commented Jan 9, 2021

alexlamsl commented Jan 9, 2021

alexlamsl commented Jan 9, 2021

alexlamsl commented Jan 9, 2021

kzc commented Jan 9, 2021

alexlamsl commented Jan 9, 2021

kzc commented Jan 9, 2021

alexlamsl commented Jan 9, 2021

alexlamsl commented Jan 9, 2021

alexlamsl commented Jan 10, 2021 • edited Loading

test.js

kzc commented Jan 10, 2021

alexlamsl commented Jan 10, 2021

kzc commented Jan 11, 2021

alexlamsl commented Jan 11, 2021

kzc commented Jan 11, 2021

alexlamsl commented Jan 11, 2021

alexlamsl commented Jan 16, 2021

alexlamsl commented Jan 16, 2021

kzc commented Jan 16, 2021

kzc commented Jan 16, 2021

alexlamsl commented Jan 16, 2021

kzc commented Jan 20, 2021

alexlamsl commented Jan 20, 2021

alexlamsl commented Jan 20, 2021

kzc commented Jan 20, 2021

alexlamsl commented Jan 20, 2021

kzc commented Jan 20, 2021

alexlamsl commented Jan 20, 2021

alexlamsl commented Jan 21, 2021

kzc commented Jan 21, 2021

kzc commented Jan 26, 2021

alexlamsl commented Jan 26, 2021

alexlamsl commented Jan 26, 2021

alexlamsl commented Jan 26, 2021

kzc commented Jan 26, 2021

alexlamsl commented Jan 26, 2021

kzc Jan 9, 2021 •

edited

Loading

kzc commented Jan 9, 2021 •

edited

Loading

alexlamsl commented Jan 10, 2021 •

edited

Loading