Add full support for sys module (debug cowboy process state)

[fishcakez]

This change provides support for the sys module in cowboy processes. Where possible new callbacks are as similar to existing OTP behaviours as possible. This changes requires 17.0 so can only be merged into the 2.0 branch, and not 1.0.

To use the debugging features added here you an use sys, observer, recon or OTP debugging tool of choice as if the cowboy processes were using the built in OTP behaviours.

Basic HTTP handlers have two new optional callbacks:

-callback code_change(any(), cowboy_req:req(), any(), any()) -> {ok, cowboy_req:req(), any()}.

-callback format_status(normal | terminate, [[{any(), any()}], cowboy_req:req(), any()]) -> any().

Middlewares and sub protocols have the following possible return values:

• {ok, Req, Env}
• {suspend, Module, Function, Args}
• {system, From, Msg, Module, Req, Env, Misc}
• {halt, Req, Env}

Notice the new system callback and the addition of Env in the halt return value.

The system return value is used to handle system messages. The Module used must implement the cowboy_system callbacks. These are as close as possible to system_* callbacks as possible:

-callback continue(cowboy_req:req(), cowboy_middleware:env(), any())
    -> {ok, cowboy_req:req(), cowboy_middleware:env()}
    | {suspend, module(), atom(), [any()]}
    | {system, {pid(), term()}, any(), module(), cowboy_req:req(), cowboy_middleware:env(), any()}
    | {halt, cowboy_req:req(), cowboy_middleware:env()}.

-callback terminate(any(), cowboy_req:req(), cowboy_middleware:env(), any()) -> no_return().

-callback code_change(cowboy_req:req(), any(), module(), any(), any()) -> {ok, cowboy_req:req(), any()}.

And may implement the three optional callbacks (again similar to the system_* callbacks):

-callback get_state(cowboy_req:req(), any()) -> {ok, {module(), cowboy_req:req(), any()}}.

-callback replace_state(replace_state(), cowboy_req:req(), any()) ->
    {ok, {module(), cowboy_req:req(), any()}, cowboy_req:req(), any()}.

-callback format_status(normal, [[{any(), any()}] |
    running | suspended | cowboy_req:req() | cowboy_middleware:env() | any()])
    -> any().

A requirement of supporting the sys module is using proc_lib. This has the following side effects:

• A crash_report is sent to the error logger on abnormal exit
• The emulator will nolonger send an error logger on uncaught error
• Uncaught errors will nolonger have a stacktrace in the exit signal

I will add more to the guide once the API has been finalised/confirmed.

[essen]

Reacting to this message only at this point.

Basic HTTP handlers have two new optional callbacks: code_change, format_status

I'm not sure what this is about. Basic HTTP handlers are uninterruptible, and have no reason to be (it's just the init/2 call and nothing else).

Notice the [...] addition of Env in the halt return value.

Why? It's not obvious why it's necessary from just this commit message.

The system return value is used to handle system messages. The Module used must implement the cowboy_system callbacks. These are as close as possible to system_* callbacks as possible:

Why not name them system_* too? Or at least something to differentiate them all. Though sys_* sounds better.

And may implement the three optional callbacks (again similar to the system_* callbacks):

Not sure why you want get_state and replace_state optional. Also I have no idea what format_status is doing/where it is used.

[fishcakez]

Basic HTTP handlers have two new optional callbacks: code_change, format_status

I'm not sure what this is about. Basic HTTP handlers are uninterruptible, and have no reason to be (it's just the init/2 call and nothing else).

Sorry I meant handlers in general! I will change the commit message to be clearer. These two functions will only be used if the handler is upgraded and behave exactly the same as the OTP behaviours, except Req is added in front of State:

• code_change is used by websockets and loop handlers to handle the change_code system message.
• format_status is used by websockets and loop handlers to handle the get_status system message.
• format_status is also used for upgraded handlers to format the state when terminating with an exception - in practise this works in a similar way to cowboy_req:to_list/1.

Notice the [...] addition of Env in the halt return value.

Why? It's not obvious why it's necessary from just this commit message.

This was so middlewares (and sub protocols) could support the sys's debug object. I will remove this change after our conversation on IRC yesterday. This will allow simplifying the cowboy_system behaviour.

The system return value is used to handle system messages. The Module used must implement the cowboy_system callbacks. These are as close as possible to system_* callbacks as possible:

Why not name them system_* too? Or at least something to differentiate them all. Though sys_* sounds better.

I had named them system_* for a while and it felt much better, However I thought in the Erlang documentation there was a note saying to only use system_* functions to handle sys:handle_system_msg - I can not find it now. I will rename cowboy_system to cowboy_sys and add the sys_* prefix. I think your suggestion is very good.

And may implement the three optional callbacks (again similar to the system_* callbacks):

Not sure why you want get_state and replace_state optional.

system_get_state and system_replace_state (and format_status) are optional callbacks, so I made the equivalent ones optional too. A default implementation is provided in OTP (and in this patch) if they are not exported. I am happy to make them required.

If those two functions are required (and Env is removed from {halt, ...} as I said above) it will allow us to remove both the Req and the Env return from {system,..}. I think this will work well and allow further simplification of the cowboy_sys behaviour so will do this change too.

Also I have no idea what format_status is doing/where it is used.

Sorry! I assumed it was clear because it works in the same as in OTP behaviours except the list provided in the second argument is slightly different. To borrow from the Erlang docs it is called in an OTP behaviour when:

• One of sys:get_status/1,2 is invoked to get the gen_event status. Opt is set to the atom normal for this case.
  • The event handler terminates abnormally and gen_event logs an error. Opt is set to the atom terminate for this case.

In the case of sys:get_status/2 the call to format_status/2 is made from the format_status/2 callback of the process implementing the behaviour (i.e. the gen_event module, or in this case the cowboy_middleware such as cowboy_websocket).

In the later case it is used to format the state in the exit reason of the process - so ranch can log an easier to read state.

As well as the changes mentioned above I will improve the commit message to explain what each callback function does.

Thank you very much for the feedback!

[essen]

There is some confusion to me about what is considered "state". When you do get_state or replace_state, do you get the handler's state?

[fishcakez]

This depends, I document that a middleware/sub_protocol should return {Module, Req, State}. The Module and State can belong to the middleware or the handler. The default is the middleware.

The initial commit has over 350 lines added to the guide and manual, some of these might be helpful for you to understand how the callbacks work. A more detailed look at these two functions is here: https://github.com/fishcakez/cowboy/blob/e7a4045dc5c5632afbed76d39bf503e588a8f974/doc/src/guide/middlewares.ezdoc#L174-L251.

The additions to the guide I mention in my opening comments is about a chapter on how to debug the handlers - rather than how to enabled a middleware/sub_protocol to be debugged, which is already in.

[essen]

For example get_state, sounds like we want {ok, Req, State} if at all possible, because that's exactly what handlers see. Also we don't need to return the module I think, since we already know it because we called the callback anyway.

[essen]

Oh but get_state is to be implemented by sub protocols I see. OK I will put more thoughts into it tomorrow.

[essen]

OK I want to cut down this PR into smaller steps, each with their docs/tests, and with some time passed in between the different merges to see if feedback comes in.

Let's start with a PR implementing system processes for middlewares and sub protocols. It will only force cowboy_sys modules to implement the sys_continue and sys_terminate callbacks. It is unclear to me what Misc is in the system tuple or how it should be used. Most middlewares have Env part of State so we should probably just have "Req and State" instead of "Req and Env and Misc/State".

Second step would be to add the mandatory sys_get_state and sys_replace_state callbacks. I would like to simplify their signature and make the return values {ok, Req, State} in all cases, with State being the middleware's. (I have no idea why replace_state is this way right now.) Same as the above functions basically, we just make them take care of a Req and State values. module() I don't think it is needed if it's going to be ?MODULE all the time, it should be Cowboy adding this info to what we return to the user.

Third step would be sys_code_change. This needs to have a large variety of tests, and it also needs to properly explain how it works with regards to handlers, what gets upgraded first, the handler, middleware? How does it work when there's more than one module involved? Is it even possible to get right? Basically a long guide chapter detailing everything.

And finally sys_format_status although I am not sure this will get much use and will need extra convincing.

I think sys_code_change and sys_format_status should be the ones that are optional as they are the ones who are likely not going to see much use, especially in handlers.

Thoughts?

[fishcakez]

This sounds good to me. I think we may need to merge first 2 steps as step
1 is not very interesting without step 2. I will attempt step 1 and 2 this
week but do two separate commits should it prove to be moving to fast.

I will remove Env from the callback arguments/return values, leaving Req
and State.

We can discuss the other points as they become relevant.

On Monday, 6 October 2014, Loïc Hoguin [email protected] wrote:

OK I want to cut down this PR into smaller steps, each with their
docs/tests, and with some time passed in between the different merges to
see if feedback comes in.

Let's start with a PR implementing system processes for middlewares and
sub protocols. It will only force cowboy_sys modules to implement the
sys_continue and sys_terminate callbacks. It is unclear to me what Misc
is in the system tuple or how it should be used. Most middlewares have
Env part of State so we should probably just have "Req and State" instead
of "Req and Env and Misc/State".

Second step would be to add the mandatory sys_get_state and
sys_replace_state callbacks. I would like to simplify their signature and
make the return values {ok, Req, State} in all cases, with State being
the middleware's. (I have no idea why replace_state is this way right
now.) Same as the above functions basically, we just make them take care of
a Req and State values. module() I don't think it is needed if it's going
to be ?MODULE all the time, it should be Cowboy adding this info to what
we return to the user.

Third step would be sys_code_change. This needs to have a large variety
of tests, and it also needs to properly explain how it works with regards
to handlers, what gets upgraded first, the handler, middleware? How does it
work when there's more than one module involved? Is it even possible to get
right? Basically a long guide chapter detailing everything.

And finally sys_format_status although I am not sure this will get much
use and will need extra convincing.

I think sys_code_change and sys_format_status should be the ones that are
optional as they are the ones who are likely not going to see much use,
especially in handlers.

Thoughts?

—
Reply to this email directly or view it on GitHub
#750 (comment).

[essen]

Alright, great. I understand about 2 being important for 1 and am not convinced they should be separated either when merging so 2 commits sounds fine.

[essen]

The format_status and proper upgrades are currently not in Cowboy itself (not tested anyway).

[essen]

Closing in favor of #1134 for tracking the remaining tasks. Thanks!