Prevent spurious /dev/shm/PostgreSQL.* files in tests
By default the postgres server uses `shm_open(3)` to create shared
memory to talk between workers. This means files directly inside of
`/dev/shm`. Since that directory is one of the few (maybe only?)
world-writeable and non-sandboxed directories, that means the server
can leave behind these useless files.
I investigated switching postgres to use the `sysv` mechanism for IPC,
but that doesn't really help us here. That just creates shared memory
segments that are even harder to track and clean up after a run.
For example:
https://www.ibm.com/docs/en/aix/7.2?topic=s-shmget-subroutine
> Once created, a shared memory segment is deleted only when the
> system reboots or by issuing the ipcrm command or using the following
> shmctl subroutine:
The final approach here is to make the test framework properly shut
down the various servers. It currently just expects the bazel sandbox
to kill the servers. I couldn't find a way to get node to
synchronously shut down the servers and wait for them to finish
shutting down. All node APIs I could find for this are asynchronous.
Those APIs cannot be used in node's `exit` hook, unfortunately. To
that end I wrote a node binding for the `waitpid(2)` syscall. This
forces node to wait for the servers to shut down before exiting the
test. This means we don't leave spurious files behind at the end of
the test in normal circumstances.
After this patch, I don't see any spurious files left behind anymore.
$ bazel test --runs_per_test=40 //scouting:scouting_test_chromium-local
Signed-off-by: Philipp Schrader <philipp.schrader@gmail.com>
Change-Id: Id798a1339cf4076c82bf31b94cc972476b57850b
diff --git a/.clang-format b/.clang-format
index fa1936b..6eb0ed4 100644
--- a/.clang-format
+++ b/.clang-format
@@ -6,6 +6,10 @@
DerivePointerAlignment: false
PointerAlignment: Right
Standard: Cpp11
+IncludeCategories:
+ # Force node headers to be considered third-party headers.
+ - Regex: '^<(node|v8).h>$'
+ Priority: 3
---
Language: JavaScript
diff --git a/scouting/testing/scouting_test_servers.py b/scouting/testing/scouting_test_servers.py
index 7a0d3c5..b6e5c7a 100644
--- a/scouting/testing/scouting_test_servers.py
+++ b/scouting/testing/scouting_test_servers.py
@@ -105,6 +105,13 @@
except FileNotFoundError:
pass
+def discard_signal(signum, frame):
+ """A NOP handler to ignore certain signals.
+
+ We use signal.pause() to wait for a signal. That means we can't use the default handler. The
+ default handler would tear the application down without stopping child processes.
+ """
+ pass
def main(argv: List[str]):
parser = argparse.ArgumentParser()
@@ -115,6 +122,8 @@
runner.start(args.port)
# Wait until we're asked to shut down via CTRL-C or SIGTERM.
+ signal.signal(signal.SIGINT, discard_signal)
+ signal.signal(signal.SIGTERM, discard_signal)
signal.pause()
runner.stop()
diff --git a/third_party/npm/@bazel/protractor/bazel-protractor.patch b/third_party/npm/@bazel/protractor/bazel-protractor.patch
index ee49a91..7c37001 100644
--- a/third_party/npm/@bazel/protractor/bazel-protractor.patch
+++ b/third_party/npm/@bazel/protractor/bazel-protractor.patch
@@ -1,8 +1,16 @@
diff --git a/node_modules/@bazel/protractor/protractor-utils.js b/node_modules/@bazel/protractor/protractor-utils.js
-index 2d577b82..c5c10c22 100755
+index 2d577b8..8d721b3 100755
--- a/node_modules/@bazel/protractor/protractor-utils.js
+++ b/node_modules/@bazel/protractor/protractor-utils.js
-@@ -67,19 +67,9 @@ var __awaiter = (this && this.__awaiter) || function (thisArg, _arguments, P, ge
+@@ -38,6 +38,7 @@ var __awaiter = (this && this.__awaiter) || function (thisArg, _arguments, P, ge
+ const runfiles = require(process.env['BAZEL_NODE_RUNFILES_HELPER']);
+ const child_process = require("child_process");
+ const net = require("net");
++ const waitpid = require("./org_frc971/tools/build_rules/js/waitpid_module/waitpid_module.node");
+ function isTcpPortFree(port) {
+ return new Promise((resolve, reject) => {
+ const server = net.createServer();
+@@ -67,19 +68,9 @@ var __awaiter = (this && this.__awaiter) || function (thisArg, _arguments, P, ge
}
exports.isTcpPortBound = isTcpPortBound;
function findFreeTcpPort() {
@@ -25,7 +33,23 @@
}
exports.findFreeTcpPort = findFreeTcpPort;
function waitForServer(port, timeout) {
-@@ -121,4 +111,4 @@ var __awaiter = (this && this.__awaiter) || function (thisArg, _arguments, P, ge
+@@ -114,6 +105,15 @@ var __awaiter = (this && this.__awaiter) || function (thisArg, _arguments, P, ge
+ throw new Error(`Server exited with error code: ${exitCode}`);
+ }
+ });
++
++ // FRC971 hack. Since postgres leaves files in /dev/shm, we want to
++ // shut it down as gracefully as possible.
++ process.on('exit', () => {
++ serverProcess.kill('SIGTERM');
++ console.log('protractor: Waiting for server to shut down.');
++ waitpid.waitpid(serverProcess.pid);
++ console.log('protractor: Server has shut down.');
++ });
+ // Wait for the server to be bound to the given port.
+ yield waitForServer(port, timeout);
+ return { port };
+@@ -121,4 +121,4 @@ var __awaiter = (this && this.__awaiter) || function (thisArg, _arguments, P, ge
}
exports.runServer = runServer;
});
diff --git a/tools/build_rules/js.bzl b/tools/build_rules/js.bzl
index f5a7543..5510266 100644
--- a/tools/build_rules/js.bzl
+++ b/tools/build_rules/js.bzl
@@ -63,7 +63,7 @@
},
)
-def protractor_ts_test(name, srcs, deps = None, **kwargs):
+def protractor_ts_test(name, srcs, deps = None, data = None, **kwargs):
"""Wraps upstream protractor_web_test_suite() to reduce boilerplate.
This is largely based on the upstream protractor example:
@@ -89,9 +89,14 @@
declaration_map = True,
)
+ data = (data or []) + [
+ "//tools/build_rules/js/waitpid_module",
+ ]
+
protractor_web_test_suite(
name = name,
srcs = [paths.replace_extension(src, ".js") for src in srcs],
deps = [":%s__lib" % name],
+ data = data,
**kwargs
)
diff --git a/tools/build_rules/js/waitpid_module/BUILD b/tools/build_rules/js/waitpid_module/BUILD
new file mode 100644
index 0000000..23fbd01
--- /dev/null
+++ b/tools/build_rules/js/waitpid_module/BUILD
@@ -0,0 +1,44 @@
+load(":defs.bzl", "collect_nodejs_headers")
+
+collect_nodejs_headers(
+ name = "nodejs_headers",
+ src = "@nodejs_linux_amd64//:npm_files",
+ target_compatible_with = [
+ "@platforms//os:linux",
+ "@platforms//cpu:x86_64",
+ ],
+)
+
+cc_binary(
+ name = "waitpid_module.so",
+ srcs = [
+ "waitpid_module.cc",
+ ":nodejs_headers",
+ ],
+ copts = [
+ "-isystem",
+ "external/nodejs_linux_amd64/bin/nodejs/include/node",
+ ],
+ linkshared = True,
+ target_compatible_with = [
+ "@platforms//os:linux",
+ "@platforms//cpu:x86_64",
+ ],
+)
+
+# Native modules have to have the .node extension. cc_binary will append .so if
+# we use `name = "waitpid_module.node"` so we manually rename the file here.
+genrule(
+ name = "rename",
+ srcs = [":waitpid_module.so"],
+ outs = [":waitpid_module.node"],
+ cmd = "cp $(SRCS) $(OUTS)",
+)
+
+filegroup(
+ name = "waitpid_module",
+ srcs = [
+ ":waitpid_module.node",
+ ],
+ visibility = ["//visibility:public"],
+)
diff --git a/tools/build_rules/js/waitpid_module/defs.bzl b/tools/build_rules/js/waitpid_module/defs.bzl
new file mode 100644
index 0000000..52d1980
--- /dev/null
+++ b/tools/build_rules/js/waitpid_module/defs.bzl
@@ -0,0 +1,28 @@
+def _collect_nodejs_headers_impl(ctx):
+ """Collect all the headers for compiling a native node module.
+
+ The @nodejs_linux_amd64 repo doesn't expose a dedicated target for the
+ nodejs headers. This rule will collect all the necessary headers. Add a
+ target of this rule to `srcs` of a `cc_binary` target.
+ """
+ files = ctx.attr.src.files.to_list()
+ headers = []
+ for file in files:
+ _, _, header = file.short_path.partition("/bin/nodejs/include/node/")
+ if not header or not header.endswith(".h"):
+ continue
+ if "/" not in header:
+ headers.append(file)
+ elif header.startswith("cppgc/"):
+ headers.append(file)
+
+ return [DefaultInfo(
+ files = depset(headers),
+ )]
+
+collect_nodejs_headers = rule(
+ implementation = _collect_nodejs_headers_impl,
+ attrs = {
+ "src": attr.label(),
+ },
+)
diff --git a/tools/build_rules/js/waitpid_module/waitpid_module.cc b/tools/build_rules/js/waitpid_module/waitpid_module.cc
new file mode 100644
index 0000000..fe5a42a
--- /dev/null
+++ b/tools/build_rules/js/waitpid_module/waitpid_module.cc
@@ -0,0 +1,67 @@
+#include <errno.h>
+#include <stdlib.h>
+#include <sys/wait.h>
+#include <unistd.h>
+
+#include <iostream>
+
+#include <node.h>
+#include <v8.h>
+
+namespace tools::build_rules::js {
+
+auto CreateString(v8::Isolate *isolate, const char *text) {
+ return v8::String::NewFromUtf8(isolate, text).ToLocalChecked();
+}
+
+void WaitPid(const v8::FunctionCallbackInfo<v8::Value> &args) {
+ v8::Isolate *isolate = args.GetIsolate();
+
+ if (args.Length() != 1 || !args[0]->IsInt32()) {
+ isolate->ThrowException(v8::Exception::TypeError(
+ CreateString(isolate, "Need a single integer argument")));
+ return;
+ }
+
+ int pid = args[0].As<v8::Int32>()->Value();
+ std::cout << "Waiting on PID " << pid << std::endl;
+
+ int status;
+
+ while (waitpid(pid, &status, 0) == -1) {
+ if (errno == EINTR) {
+ continue;
+ }
+ isolate->ThrowException(v8::Exception::Error(
+ CreateString(isolate, "waitpid() failed")));
+ return;
+ }
+
+ // The expectation is for the child process to exit with 0. Anything else is
+ // an error that can be debugged separately.
+ if (WIFEXITED(status)) {
+ if (WEXITSTATUS(status) != 0) {
+ isolate->ThrowException(v8::Exception::Error(
+ CreateString(isolate, "child exited with error code")));
+ return;
+ }
+ } else if (WIFSIGNALED(status)) {
+ isolate->ThrowException(v8::Exception::Error(
+ CreateString(isolate, "child exited because of signal")));
+ return;
+ } else {
+ isolate->ThrowException(v8::Exception::Error(
+ CreateString(isolate, "unhandled child state change")));
+ return;
+ }
+
+ std::cout << "Successfully waited on PID " << pid << std::endl;
+}
+
+void Initialize(v8::Local<v8::Object> exports) {
+ NODE_SET_METHOD(exports, "waitpid", WaitPid);
+}
+
+NODE_MODULE(waitpid_module, Initialize)
+
+} // namespace tools::build_rules::js