Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Handle request processes exits that get sent exit signal #1511

Open
hubertlepicki opened this issue Apr 21, 2021 · 4 comments
Open

Handle request processes exits that get sent exit signal #1511

hubertlepicki opened this issue Apr 21, 2021 · 4 comments

Comments

@hubertlepicki
Copy link

hubertlepicki commented Apr 21, 2021

When a web request is being processes by Cowboy, and the request handling process get sent exit signal (as in with exit/2), it seems like the process is silently crashing without reporting anything to the logger.

It looks like Cowboy handles exits of these processes here using try/catch clause:

https://github.com/ninenines/cowboy/blob/master/src/cowboy_stream_h.erl#L289-L300

But it does only work if process exits on it's own. When it is being sent exit signal, it's not.

This is because the process is not trapping exits, and even that wouldn't catch all signals, i.e. kill wouldn't get detected.

I have to detect these exits, and I am currently doing that by setting up a monitor.

I am just wondering if that is not something Cowboy would want to be doing internally? Since it catches exits of these processes if they exit with exit/1, maybe it makes sense to add functionality to catch abnormal exits when these processes are sent a signal with exit/2 too?

@essen
Copy link
Member

essen commented Apr 21, 2021

What you linked to is only to add the stacktrace to exit reasons. The actual handling is at https://github.com/ninenines/cowboy/blob/master/src/cowboy_stream_h.erl#L127-L156

You are correct that it's missing a more general clause.

@hubertlepicki
Copy link
Author

Right, so it's probably missing the clause of

info(StreamID, Exit={'EXIT', Pid, Reason})

Want me to give it a shot with a PR?

@essen
Copy link
Member

essen commented Apr 22, 2021

Sure.

@hubertlepicki
Copy link
Author

I have created a PR. There's at least one issue with it (silencing crashes in tests that I don't know how to do properly). Other than that it seems to resolve the issue at least on my app.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants