[#1551] Handle external exits of processes #1514
Conversation
Adds handler to capture external exits of processes, and tests to verify this is indeed happening. Problem: I don't know how to silence crash reports in this case. When I run the tests, I can see the crash reports I added, but I would like to silence them in tests. ct_helper:ignore() doesn't seem to cut it the way I use it, which is probably wrong. I see on the console: Testing ninenines.cowboy.metrics_SUITE: Starting test, 96 test cases =ERROR REPORT==== 23-Apr-2021::11:22:09 === Ranch listener http, connection process <0.266.0>, stream 1 had its request process <0.275.0> exit with reason external_exit which we probably want to silence but I'm not sure how. Help?
hubertlepicki
changed the title
| @@ -154,6 +154,22 @@ info(StreamID, Exit={'EXIT', Pid, {Reason, Stacktrace}}, State=#state{ref=Ref, p | |||
| do_info(StreamID, Exit, [ | |||
| {error_response, 500, #{<<"content-length">> => <<"0">>}, <<>>} | |||
| |Commands], State); | |||
| info(StreamID, Exit={'EXIT', Pid, Reason}, State=#state{ref=Ref, pid=Pid}) -> | |||
| Commands0 = [{internal_error, Exit, 'Stream process crashed.'}], | |||
There was a problem hiding this comment.
Let's use exited rather than crashed.
| end, | ||
| do_info(StreamID, Exit, [ | ||
| {error_response, 500, #{<<"content-length">> => <<"0">>}, <<>>} | ||
| |Commands], State); |
There was a problem hiding this comment.
You can probably extract all this into a separate function, with slightly different behavior depending on the presence of a stacktrace (use undefined for the exit case).
| exit(internal_exit); | ||
| init(_, external_exit) -> | ||
| ct_helper:ignore(?MODULE, init, 2), | ||
| exit(self(), external_exit). |
There was a problem hiding this comment.
The ignore function is about crashes not exits. This handler is about crashes as well. I don't think this belongs here. A better place for this test would be in stream_handler_SUITE similar to this test https://github.com/ninenines/cowboy/blob/master/test/stream_handler_SUITE.erl#L341-L362 (just get the pid and exit the process immediately, the test shouldn't be that long). It's OK to only test exit/2 in this case.
| ref := _, | ||
| pid := From, | ||
| streamid := 1, | ||
| reason := {internal_error, {'EXIT', _Pid, external_exit}, 'Stream process crashed.'}, |
There was a problem hiding this comment.
That said, checking this still seems valuable. So I would leave this test. I would just do the exit from the test case instead.
There was a problem hiding this comment.
alright makes sense. On it.
Relates to #1511
Adds handler to capture external exits of processes, and tests to verify
this is indeed happening.
On my Elixir/Phoenix project, where I add such line in a controller to kill the process:
Without this change, no exception or error is reported to the logger.
With these changes I see the error line as I'd expect:
There is at least one one problem: I don't know how to silence crash reports in this case. When I run the tests, I can see the crash reports I added, but I would like to silence them in tests. ct_helper:ignore() doesn't seem to cut it the way I use it, which is probably wrong. I see on the console:
which we probably want to silence but I'm not sure how, so a suggestion how and a general review of my code would be very much appreciated. Unfortunately I don't write Erlang daily so I appreciate feedback.