src: make AtExit() callbacks run in reverse order

addaleax · MylesBorins · commit 2e729f2dc9da · 2019-12-17T17:06:14.000-05:00
This makes the actual behaviour match the documented (and arguably the correct) behaviour. PR-URL: #30230 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com> Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>
diff --git a/doc/api/addons.md b/doc/api/addons.md
@@ -1354,10 +1354,10 @@ static void sanity_check(void*) {
 }
 
 void init(Local<Object> exports) {
+  AtExit(sanity_check);
   AtExit(at_exit_cb2, cookie);
   AtExit(at_exit_cb2, cookie);
   AtExit(at_exit_cb1, exports->GetIsolate());
-  AtExit(sanity_check);
 }
 
 NODE_MODULE(NODE_GYP_MODULE_NAME, init)
diff --git a/src/env.cc b/src/env.cc
@@ -636,7 +636,7 @@ void Environment::RunAtExitCallbacks() {
 }
 
 void Environment::AtExit(void (*cb)(void* arg), void* arg) {
-  at_exit_functions_.push_back(ExitCallback{cb, arg});
+  at_exit_functions_.push_front(ExitCallback{cb, arg});
 }
 
 void Environment::RunAndClearNativeImmediates() {
diff --git a/test/cctest/test_environment.cc b/test/cctest/test_environment.cc
@@ -10,8 +10,12 @@ using node::RunAtExit;
 
 static bool called_cb_1 = false;
 static bool called_cb_2 = false;
+static bool called_cb_ordered_1 = false;
+static bool called_cb_ordered_2 = false;
 static void at_exit_callback1(void* arg);
 static void at_exit_callback2(void* arg);
+static void at_exit_callback_ordered1(void* arg);
+static void at_exit_callback_ordered2(void* arg);
 static std::string cb_1_arg;  // NOLINT(runtime/string)
 
 class EnvironmentTest : public EnvironmentTestFixture {
@@ -20,6 +24,8 @@ class EnvironmentTest : public EnvironmentTestFixture {
     NodeTestFixture::TearDown();
     called_cb_1 = false;
     called_cb_2 = false;
+    called_cb_ordered_1 = false;
+    called_cb_ordered_2 = false;
   }
 };
 
@@ -61,6 +67,19 @@ TEST_F(EnvironmentTest, AtExitWithoutEnvironment) {
   EXPECT_TRUE(called_cb_1);
 }
 
+TEST_F(EnvironmentTest, AtExitOrder) {
+  const v8::HandleScope handle_scope(isolate_);
+  const Argv argv;
+  Env env {handle_scope, argv};
+
+  // Test that callbacks are run in reverse order.
+  AtExit(*env, at_exit_callback_ordered1);
+  AtExit(*env, at_exit_callback_ordered2);
+  RunAtExit(*env);
+  EXPECT_TRUE(called_cb_ordered_1);
+  EXPECT_TRUE(called_cb_ordered_2);
+}
+
 TEST_F(EnvironmentTest, AtExitWithArgument) {
   const v8::HandleScope handle_scope(isolate_);
   const Argv argv;
@@ -134,3 +153,13 @@ static void at_exit_callback1(void* arg) {
 static void at_exit_callback2(void* arg) {
   called_cb_2 = true;
 }
+
+static void at_exit_callback_ordered1(void* arg) {
+  EXPECT_TRUE(called_cb_ordered_2);
+  called_cb_ordered_1 = true;
+}
+
+static void at_exit_callback_ordered2(void* arg) {
+  EXPECT_FALSE(called_cb_ordered_1);
+  called_cb_ordered_2 = true;
+}

Original file line number	Diff line number	Diff line change
`@@ -1354,10 +1354,10 @@ static void sanity_check(void*) {`
`1354`	`1354`	`}`
`1355`	`1355`
`1356`	`1356`	`void init(Local<Object> exports) {`
	`1357`	`+ AtExit(sanity_check);`
`1357`	`1358`	`AtExit(at_exit_cb2, cookie);`
`1358`	`1359`	`AtExit(at_exit_cb2, cookie);`
`1359`	`1360`	`AtExit(at_exit_cb1, exports->GetIsolate());`
`1360`		`- AtExit(sanity_check);`
`1361`	`1361`	`}`
`1362`	`1362`
`1363`	`1363`	`NODE_MODULE(NODE_GYP_MODULE_NAME, init)`
Original file line number	Diff line number	Diff line change
`@@ -636,7 +636,7 @@ void Environment::RunAtExitCallbacks() {`
`636`	`636`	`}`
`637`	`637`
`638`	`638`	`void Environment::AtExit(void (cb)(void arg), void* arg) {`
`639`		`- at_exit_functions_.push_back(ExitCallback{cb, arg});`
	`639`	`+ at_exit_functions_.push_front(ExitCallback{cb, arg});`
`640`	`640`	`}`
`641`	`641`
`642`	`642`	`void Environment::RunAndClearNativeImmediates() {`