Consider using absl::Notification instead of implementing your own version.

Here you could call n.Notify();
You would call n.WaitForNotification() before code that depends on tid.

Done.

I assume there's no specific string that we can assert in the EXPECT_DEATH

Does Envoy install a failure signal handler? Should we consider testing by installing a signal handler for SIGABRT that records which thread received the SIGABRT signal?

Envoy installs failure handlers; it does seem that under different compilation modes this test will report different failure strings -- specifically under ASAN it seems to trigger some asan specific code that isn't triggered in the other test below or when running this test in other models (ASAN:DEADLYSIGNAL is part of the print out)

I can try to add some extra logic in if you think it'd be important to figure out the exact string that caused death.

It seems to me that right now you can't distinguish between death due to the kill signal vs the call to panic. The kill signal code could be removed and this test would still pass.

You may want to restore signal handlers in the test destructor.

since sigactions are done within EXPECT_DEATH and we end up forking, it doesn't affect the test suite process but rather the child process which will end up dying, so we shouldn't need to restore signal handlers.

There are 3 reasons I think this would be handy (vs deferring to the underlying watchdog default actions):

• Easier to see where the failure occurred, and that signaling failed without having to jump around as much to understand the behavior.
• It's also keeps this self-contained if there were changes on the underlying system behavior in watchdog kill / multikill default
• Allows flexibility for using this in places where there isn't a default PANIC afterwards

Yep, I think I'll keep it simple for now given that the default envoy handlers views SIGABRT as a fatal signal.

The terminology of these two get a bit messy; in the docs and at a high level we talk about WatchDog and the Watch dog system, while in the actual implementation we have a watchdog per thread and a guarddog that manages the watchdogs and will actually be executing these functions.

I went with Watchdog since it seemed more friendly to folks who aren't digging down into the implementation details.

since sigactions are done within EXPECT_DEATH and we end up forking, it doesn't affect the test suite process but rather the child process which will end up dying, so we shouldn't need to restore signal handlers.

Done.

Envoy installs failure handlers; it does seem that under different compilation modes this test will report different failure strings -- specifically under ASAN it seems to trigger some asan specific code that isn't triggered in the other test below or when running this test in other models (ASAN:DEADLYSIGNAL is part of the print out)

I can try to add some extra logic in if you think it'd be important to figure out the exact string that caused death.

Is this call to signal(SIGABRT, SIG_IGN); required?

Not strictly, but it prevents subsequent printouts of Eating signal:

Comment seems wrong/out of date. Note that this comment is the primary documentation for this in the generated docs. It should include some information about what this does, when/why use it, etc.

Done. Added additional information from the action's c++ implementation as you suggested.

Perhaps some/all of this comment block should be included with the proto config, so it gets included in generated docs.

Done.

Should this be the default, on supported platforms? Is there any downside to this? It seems to me that it gives better information (or a chance of it), and the same net behavior (process terminated). @envoyproxy/maintainers

I'm fine with that being default in a future PR.

But also fine here if we want this, seems generally useful.

Sounds good, I can make it be a default action in a future PR.

The main downside I see of making it a default now would be different behaviors across platforms due to a lack of support on Windows. Should we wait for parity before we make it a default?

I think it's ok to make it the default, because the only platform difference should be diagnostic output. The process is terminated on all platforms.

I'm also fine with making it the default in a future PR. It's your choice, @KBaichoo .

Sounds good. I'll submit a follow up to this adding this action to the watchdog actions by default.

+1 to make this the default.

Is this actually needed? I think Windows has its own extension bzl file so I'm not sure this is even compiled? Could we actually avoid the ifdef in here now or just replace with an proprocessor error if this extension gets compiled on windows?

Good to know this is a possibility, thanks for pointing it out.

I've added the extension to WINDOWS_SKIP_TARGETS which IIUC will let it be skipped by windows so I can remove the ifdef #Win32 from code.

turns out you also need to add tags = ["skip_on_windows"] on tests since excluding the extension in the .bzl doesn't effect the tests.

I haven't kept fully up to date on this, but isn't the default action at the end of the action chain to terminate the process? Do we need this or would the action chain just end up aborting anyway?

Yes, you're right. I discussed this with antonio up above. I've copied the reasoning below (copied from #12860 (comment))

There are 3 reasons I think this [having a panic in the action] would be handy (vs deferring to the underlying watchdog default actions[its panic there]):

• Easier to see where the failure occurred, and that signaling failed without having to jump around as much to understand the behavior.
• It's also keeps this self-contained if there were changes on the underlying system behavior in watchdog kill / multikill default
• Allows flexibility for using this in places where there isn't a default PANIC afterwards

Given that we're planning to make this also a default action on platform where it's supported I'd likely make the following change in the next PR:

• If we're doing kill or multikill, then install the abort action as the final watchdog action for either of those events.

I could possibly see when this has full support across platforms that we'd remove the watchdog's default panic, since it'd end up being dead code.

OK that make sense. Can you summarize this comment in the code? Thank you.

/wait

Done.