Add basic support for system messages

[fishcakez]

This changes performs the first step of #750, a simplified version with much improved testing. All system messages are handled by cowboy_protocol or cowboy_middleware. Middlewares, sub_protocols (and handlers) can not change how system messages are handled.

To have cowboy_middleware handle a system message a middleware (or sub_protocol) returns:

{system, From, Msg, Module, Req, State}

Once the system message(s) have been handled control is passed back to the middleware via a cowboy_sys callback. Either to continue processing as before:

sys_continue(Req, #state{env=Env}) ->
    {ok, Req, Env}.

Or to terminate with reason Reason:

sys_terminate(Reason, _Req, _State) ->
    exit(Reason).

For more information please see the commit message or the additions to the guide.

Help is required to test handling of system messages when a websocket handler receives the message in the websocket_payload_loop receive clause.

[fishcakez]

@essen, I hope this is what you intended for step 1. I did not do step 2 as this step is interesting on its own and more than enough to be getting on with and gaining feedback.

[essen]

Sounds like it, will look when I get back tonight.

[essen]

Just saying once but all cowboy must be Cowboy unless it's the atom specifically.

[essen]

My only issue (besides that it needs rebasing) is that 'shutdown' needs to be renamed to 'stop'.

Then I will merge it.

[fishcakez]

In 18.0 there is a new function sys:terminate. This requires a test case and doc changes. I think such a test will fail in one situation so I may need to fix that too. Would you prefer I add a second commit before rebasing or squash and rebase? I will try to handle this before the weekend.

[essen]

Actually I attempted to do a merge myself. Then decided against it. I now have a lot of doubts and questions about everything.

If that's OK with you I would like to postpone that a little, and perhaps split some things out of it while waiting. Some things I can see merging immediately without question, like the change of erlang:Class into exit.

I mostly have a lot of doubts around proc_lib, sys, cowboy_proc and cowboy_sys. It just doesn't feel right. That's probably because I don't have the full picture, and perhaps merging with small steps will help me cast those doubts away. I also do not really understand why we end up with cowboy_proc and cowboy_sys instead of just cowboy_sys, we don't have to mirror OTP I think, it just sounds like a waste.

If that's OK I will move onto other breaking changes, like using maps for options. I also wanted to see if I could unify cowboy_sub_protocol into cowboy_middleware to simplify things around. You should still be able to send the erlang:Class->exit PR and perhaps other small things I didn't see meanwhile.

I think the merge should go like this:

• First the small things like using exit, to get them out of the way
• Then handle exit messages from parent perhaps.
• Then use proc_lib to spawn processes
• Then system messages?

[fishcakez]

Sure, ok. I'll do one of those bullet points at a time and work on the next only once one gets merged in.

[essen]

Yep that's the only way I can see making this work. :-) Thanks!

[essen]

@fishcakez Revisiting this.

We can not use sys without proc_lib[1][2]. Notice in [2] that when a process is sys suspended it will hibernate using proc_lib and so after hibernation we will be inside proc_lib try..catch and will generate a crash_report on abnormal exit.

I'm confused at this statement. The hibernate argument comes from here: https://github.com/erlang/otp/blob/maint/lib/stdlib/src/sys.erl#L349 and is only available through an undocumented function (defaults to false). This function is also only called by the process being suspended, so it would not be too difficult for Cowboy to simply not hibernate on suspend (especially since we don't allow the user direct access to proc_lib/sys).

Therefore I don't see any barriers to implementing a thin proc_lib equivalent that would work for Cowboy's purposes. All we have to do is to not call this function with hibernate set to true. And if one day we want to suspend+hibernate, we will simply special case it (a bit like what code_server does, except only for a single case).

Did I miss anything?

[fishcakez]

I was wrong and you didn't miss anything!

proc_lib sets up some process dictionary entries that are used by proc_lib
spawned children and by debugging tools, $initial_call and $ancestors. This
may mean debug and crash_report information is not quite as good as it
might be. However I don't think $initial_call will be an issue because the
real initial call will be used. For $ancestors it will mean the ancestors
entry is missing for any "fully compliant" OTP spawned children with debug
tools and logged crash_report. If you wanted cowboy could set these values
in the process dictionary.

I said "fully compliant" above because what you suggest wouldn't follow
exactly what OTP does or says to do. However that is a pedantic view. As
long as cowboy sends its own version of a crash_report when proc_lib would
then it sounds good to me.

On 4 May 2016 at 11:34, Loïc Hoguin [email protected] wrote:

@fishcakez https://github.com/fishcakez Revisiting this.

We can not use sys without proc_lib[1][2]. Notice in [2] that when a
process is sys suspended it will hibernate using proc_lib and so after
hibernation we will be inside proc_lib try..catch and will generate a
crash_report on abnormal exit.

I'm confused at this statement. The hibernate argument comes from here:
https://github.com/erlang/otp/blob/maint/lib/stdlib/src/sys.erl#L349 and
is only available through an undocumented function (defaults to false).
This function is also only called by the process being suspended, so it
would not be too difficult for Cowboy to simply not hibernate on suspend
(especially since we don't allow the user direct access to proc_lib/sys).

Therefore I don't see any barriers to implementing a thin proc_lib
equivalent that would work for Cowboy's purposes. All we have to do is to
not call this function with hibernate set to true. And if one day we want
to suspend+hibernate, we will simply special case it (a bit like what
code_server does, except only for a single case).

Did I miss anything?

—
You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub
#754 (comment)

[essen]

I intend to replicate everything OTP does, minus the requirement to have SASL running to see crash logs. The problem is that with the current code, when something crashes nothing is printed, and that's damn confusing. I don't rule out having an option to enable strict OTP behavior though, I just think we should be verbose by default (but not too verbose like when SASL is enabled, just the same amount as in 1.0).

Thanks for your reply!

[fishcakez]

OTP doesn't require SASL to be running to see crash logs, though I think all the handlers that handle crash_report's in OTP are in SASL. I think that is what you meant though.

When I saying "fully compliant" I am putting in quotes because I am suggesting you would be getting very similar behaviour but not using proc_lib as OTP's manual says to use for special processes and as all special processes in OTP use.

If the crash_report's are too verbose wouldn't it be better to provide a custom error_logger handler to log only a subset of the crash_reports? Or add this feature to SASL? Perhaps it could be a list of fields to include. If an error does occur why would there be no log? The emulator generated log should occur for exceptions of error or throw that aren't caught when not using proc_lib.

[essen]

Actually as I wrote a reply I realized I misidentified the issue. It's more complex than just good old proc_lib/sys. I'll get back with more info in a few days, thanks for the brainstorming. proc_lib/sys might be just fine.

[essen]

I think this is my issue with it:

6> spawn_link(fun() -> ssl:send(a,b) end).       
<0.43.0>
7> flush().                               
=ERROR REPORT==== 3-Jun-2016::10:12:02 ===
Error in process <0.43.0> with exit value:
{function_clause,[{ssl,send,[a,b],[{file,"ssl.erl"},{line,275}]}]}

Shell got {'EXIT',<0.43.0>,
              {function_clause,
                  [{ssl,send,[a,b],[{file,"ssl.erl"},{line,275}]}]}}
ok
8> proc_lib:spawn_link(fun() -> ssl:send(a,b) end).
<0.46.0>
9> flush().                                        
Shell got {'EXIT',<0.46.0>,function_clause}

This makes it harder to get meaningful information about an error (to introspect programmatically and decide what to do, for example). Maybe I'm missing something. Breakfast first though.

[essen]

Might be just a case of proc_lib needing to call 'raise' rather than 'exit', though I'm unsure if they want to call 'exit' on purpose or if 'error' would be also fine.

[essen]

I am working on a patch making proc_lib behave like normal processes in that regard. Only difference is of course that the stacktrace changes slightly.

[fishcakez]

If you search for nocatch in this PR it will show how to handle the exit reasons, and have to use exit/1. Then stacktrace is same as elsewhere. Maybe you have discovered this already.

Such a PR will break some of my code :(

[essen]

Yeah but nocatch is basically a hack and the exit signal remains different from spawn_link. I don't think there's any argument in favor of making proc_lib behave differently, it's probably like this because there was no way to reraise an exception before.

I know that kind of patch can break code so I'm working on it against master. :-) And I doubt it'll get in before OTP 20.

[fishcakez]

If raise is used then the emulator generated log message will be done for
error and throw. The only way to avoid that is to use exit hack.

On Friday, 3 June 2016, Loïc Hoguin [email protected] wrote:

Yeah but nocatch is basically a hack and the exit signal remains different
from spawn_link. I don't think there's any argument in favor of making
proc_lib behave differently, it's probably like this because there was no
way to reraise an exception before.

I know that kind of patch can break code so I'm working on it against
master. :-) And I doubt it'll get in before OTP 20.

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#754 (comment), or mute
the thread
https://github.com/notifications/unsubscribe/AB6JTQXqY054DViEkpCmuTMCd1grEANMks5qH_5WgaJpZM4CtScL
.

[essen]

I think all this is in.