Mega PR: unhandledRejection, uncaughtException, reliable IO capture, consistent tests. #206

jamestalmage · 2015-11-13T10:27:20Z

Combination of other PR's: #211, #213, #215

It adds the following features:

Reporting of uncaught exceptions in forked processes.
Reporting of unhandled promise rejections in forked processes.
Increased likelihood of receiving and printing all data from forked process stdout and stderr.
Improved reliability of stdout/stderr capture in AppVeyor CI by detecting AppVeyor and slowing down process.exit events.
Since the above solution is not perfect, increase likelihood of passing tests on AppVeyor by running the tests a second time if they fail.

I have some ideas for refactoring that might make some of this cleaner, but I know there are a number of pending PR's, and I don't want to go crazy until those land.

sindresorhus · 2015-11-15T17:56:56Z

cli.js

 	// TODO: figure out why this needs to be here to
 	// correctly flush the output when multiple test files
 	process.stdout.write('');
+	process.stderr.write('');


Wut, another one? :p

Yeah, there's 3 of process.stdout.write('') now for no explained reason.

oops - I meant to delete that other one.

There should be one each stderr and stdout. This belongs in the "it seamed to help" pile. Definitely room to experiment with removing it down the road.

sindresorhus · 2015-11-15T18:03:48Z

Why combine the unhandledRejection/uncaughtException stuff with the rest? Seems it could be a separate PR? Or is there a coupling with the other fixes?

sindresorhus · 2015-11-15T18:07:39Z

lib/fork.js


+	var endPromise = Promise.all([promise.reflect(), stdout, stderr]).then(function () {


This is weird. Why not just wait on ps.on('exit')?

I've come at this at least 5 times, and every time, it has required all these fixes.

I don't know that I've ever applied this one last, so it could be unnecessary. I can pull it out with another commit and see what happens.

#219 will tell us if this was necessary.

sindresorhus · 2015-11-15T18:13:42Z

// @vdemedes @floatdrop

jamestalmage · 2015-11-15T21:08:07Z

Why combine the unhandledRejection/uncaughtException stuff with the rest? Seems it could be a separate PR?

The uncaught exception stuff is definitely coupled. These all started as separate PR's (see description).

jamestalmage · 2015-11-15T21:13:14Z

My preference would be to get this merged so everybody can start getting green PR's again. I can then work backwards and see what can be pulled out. Working backwards from green is going to be easier than going the other direction.

Tests are just too brittle on AppVeyor. `npm install` fails frequently, and Node on Windows does not seem to reliably flush stdout/stderr of forked child processes. I don't like this, but I'm at a loss for other solutions.

wait for forked streams to end before resolving promise (cherry picked from commit 4539ccc) wait for io streams to end even when fork promise is rejected (cherry picked from commit 6bff82a)

add small delay on exit from babel (cherry picked from commit 11961f7) if AVA is not required in a test, throw an error instead of process.exit only extend timeout if env.AVA_APPVEYOR is set, so we only slow down in our tests.

jamestalmage · 2015-11-16T06:20:15Z

Just Sayin...

jamestalmage · 2015-11-16T06:22:46Z

@sindresorhus
Every comment except the separating out loud-rejection promises is addressed.

I think I can pull it out if you really think it is necessary. I think we will still have reliable builds without it.

floatdrop · 2015-11-16T07:03:43Z

Great work! - reliable builds on Windows is must have. Rough edges can be improved from there.

I'm think that unhandled related code (and serialize-value replacement too?) --should-- could be moved in separate PR - it will be easier to find corresponding code to fixes later. Not critical thou.

jamestalmage · 2015-11-16T07:34:34Z

serialize-value is part of the fix for uncaught exceptions, and that is part of the solution for reliable builds. Allowed to go uncaught, they cause the child to exit unpredictably (we are not sure exactly what messages the parent has received). With this we notify parent "hey, uncaught error here, bailing!", wait for acknowledgement, and then shut down. Otherwise we must rely on stderr / stdout from the child to be properly piped to stderr/stdout on the parent. For reasons unknown to me, that is just too unreliable on Windows to be a solution.

Unfortunately, I committed the unhandled-rejection stuff before the uncaught-exception, and there a number of conflicts between the two (here, here, etc.). Part of the frustration was that everything passed on my machine, yet failed on AppVeyor. Scroll through the AppVeyor history if you wanna see my name 3 dozen times next to a lot of red 😠. It is probably not a huge deal to pull out the unhandled rejection stuff, but there will be merge conflicts to resolve before and after, and it just seems like extra work for not a lot of benefit.

I think I did do a pretty good job maintaining a good git history, and have squashed relevant commits together. Perhaps do not squash this PR so git blame and the commit log maintain finer grained details for each change?

floatdrop · 2015-11-16T08:38:06Z

@jamestalmage no offence at all (sorry if it seems so, English is not my strongest part), really appreciate work you done in ava and related modules. I understand, that rebasing unhandledRejection is hard and not insisting on it. Thanks for explaining serialize-value part.

jamestalmage · 2015-11-16T08:46:58Z

no offence

none taken 😄

The only thing that has offended me during this process is Windows!

sindresorhus · 2015-11-16T10:32:17Z

test/test.js

+	execCli('fixture/uncaught-exception.js', function (err, stdout, stderr) {
+		t.ok(err);
+		t.ok(/Can't catch me!/.test(stderr));
+		// TODO: This should get printed, but we reject the promise (ending all tests) instead of just ending that one test and reporting.


For TODO's I prefer to include the username so we know who added it without having to git blame.

TODO(jamestalmage):

cool, I'm assuming you took care of it in the merge? Or do you want me to go handle it now?

I'll do it.

sindresorhus · 2015-11-16T10:36:54Z

Looks good to me. Let's get this merged asap. Just waiting for @vdemedes to review.

vadimdemedes · 2015-11-16T10:40:03Z

lib/fork.js

@@ -18,6 +18,10 @@ module.exports = function (args) {

 	var ps = childProcess.fork(babel, args, options);

+	function send(command, data) {
+		ps.send({'ava-child-process-command': command, 'data': data});


can just be:

ps.send({ name: command, data: data });

My only thought there was wether or not we wanted to monkeypatch process.on on the child process, so users never saw our communication.

I can imagine a scenario where you are writing some code that might be a child process (or might be top level), and so they would will listen to process.on('message'). By choosing a unique property name here we could monkeypatch on and filter out these messages. I reemit these on the child process as specific ava- prefixed events(process.on('ava-kill', ...), process.on('ava-cleanup'), etc) - that it is really unlikely users will listen to.

None of that is implemented. Just leaving the door open.

Mega PR: unhandledRejection, uncaughtException, reliable IO capture, consistent tests.

sindresorhus · 2015-11-16T11:21:35Z

Landed! Superb work tracking down all these issues @jamestalmage :)

I would honestly have just given up and removed AppVeyor :p

jamestalmage · 2015-11-16T11:26:38Z

Awesome!

I would honestly have just given up and removed AppVeyor :p

Now you tell me!! 😉

sindresorhus · 2015-11-16T11:28:24Z

Now you tell me!! 😉

Automated Windows testing is especially important for a test runner, though, so glad we have it, regardless of what I feel about Windows.

This was referenced Nov 13, 2015

Tracking Issue for AppVeyor failures. #197

Closed

(DO NOT MERGE): Reliable output #213

Closed

jamestalmage force-pushed the use-loud-rejection branch 4 times, most recently from 6585856 to cfbba40 Compare November 15, 2015 04:48

This was referenced Nov 15, 2015

Make AppVeyor retry failed runs. #215

Closed

wait for forked streams to end before resolving promise #211

Closed

jamestalmage changed the title ~~wip - Report unhandledRejections~~ Mega PR: unhandledRejection, uncaughtException, reliable IO capture, consistent tests. Nov 15, 2015

sindresorhus reviewed Nov 15, 2015
View reviewed changes

jamestalmage mentioned this pull request Nov 15, 2015

DO NOT MERGE: mega less IO promises #219

Closed

make AppVeyor retry failed runs.

a73374d

Tests are just too brittle on AppVeyor. `npm install` fails frequently, and Node on Windows does not seem to reliably flush stdout/stderr of forked child processes. I don't like this, but I'm at a loss for other solutions.

jamestalmage force-pushed the use-loud-rejection branch from cfbba40 to 9611e2f Compare November 16, 2015 04:49

report unhandled promise rejections

862edb8

jamestalmage force-pushed the use-loud-rejection branch from 9611e2f to 7ef0503 Compare November 16, 2015 05:28

jamestalmage added 2 commits November 16, 2015 00:35

report uncaught exceptions

c5d02f1

make shure IO streams are flushed before exiting a process.

b3bc42a

wait for forked streams to end before resolving promise (cherry picked from commit 4539ccc) wait for io streams to end even when fork promise is rejected (cherry picked from commit 6bff82a)

jamestalmage force-pushed the use-loud-rejection branch from 7ef0503 to a156f25 Compare November 16, 2015 05:36

extend exit delay on AppVeyor

15bd794

add small delay on exit from babel (cherry picked from commit 11961f7) if AVA is not required in a test, throw an error instead of process.exit only extend timeout if env.AVA_APPVEYOR is set, so we only slow down in our tests.

jamestalmage force-pushed the use-loud-rejection branch from 29d55cc to 15bd794 Compare November 16, 2015 06:18

sindresorhus reviewed Nov 16, 2015
View reviewed changes

vadimdemedes reviewed Nov 16, 2015
View reviewed changes

sindresorhus added a commit that referenced this pull request Nov 16, 2015

Merge pull request #206 from jamestalmage/use-loud-rejection

b24b2ea

Mega PR: unhandledRejection, uncaughtException, reliable IO capture, consistent tests.

sindresorhus merged commit b24b2ea into avajs:master Nov 16, 2015

jamestalmage deleted the use-loud-rejection branch November 16, 2015 11:25

jugglinmike mentioned this pull request Sep 16, 2017

Include anonymous functions in stacktraces #1508

Merged


		var endPromise = Promise.all([promise.reflect(), stdout, stderr]).then(function () {

Mega PR: unhandledRejection, uncaughtException, reliable IO capture, consistent tests. #206

Mega PR: unhandledRejection, uncaughtException, reliable IO capture, consistent tests. #206

Uh oh!

Conversation

jamestalmage commented Nov 13, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sindresorhus commented Nov 15, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sindresorhus commented Nov 15, 2015

Uh oh!

jamestalmage commented Nov 15, 2015

Uh oh!

jamestalmage commented Nov 15, 2015

Uh oh!

jamestalmage commented Nov 16, 2015

Uh oh!

jamestalmage commented Nov 16, 2015

Uh oh!

floatdrop commented Nov 16, 2015

Uh oh!

jamestalmage commented Nov 16, 2015

Uh oh!

floatdrop commented Nov 16, 2015

Uh oh!

jamestalmage commented Nov 16, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sindresorhus commented Nov 16, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sindresorhus commented Nov 16, 2015

Uh oh!

jamestalmage commented Nov 16, 2015

Uh oh!

sindresorhus commented Nov 16, 2015

Uh oh!

Uh oh!