sys protocol in websocket handler #454

Open
maxlapshin opened this Issue Feb 27, 2013 · 13 comments

Comments

Projects
None yet
3 participants
Contributor

maxlapshin commented Feb 27, 2013

I've got bad situation with leaking websocket handler:

(flussonic@127.0.0.1)5> sys:get_status(pid(0,30511,29)).
22:38:01.936 <0.30511.29> api_handler:272 api_websocket msg: {system,{<0.30811.81>,#Ref<0.0.163.218563>},get_status}
** exception exit: {timeout,{sys,get_status,[<0.30511.29>]}}
in function sys:send_system_msg/2 (sys.erl, line 231)
(flussonic@127.0.0.1)6>
(flussonic@127.0.0.1)6> process_info(pid(0,30511,29)).
{current_function,{cowboy_websocket,handler_loop,4}},
{initial_call,{cowboy_protocol,init,4}},
{status,waiting},
{message_queue_len,0},
{messages,[]},
{links,[<0.188.0>,<0.196.0>,#Port<0.669854>]},
{dictionary,[]},
{trap_exit,false},
{error_handler,error_handler},
{priority,normal},
{group_leader,<0.89.0>},
{total_heap_size,2514875},
{heap_size,832040},
{stack_size,14},
{reductions,272853732},
{garbage_collection,[{min_bin_vheap_size,46368},
{min_heap_size,233},
{fullsweep_after,65535},
{minor_gcs,6}]},
{suspending,[]}
7> process_info(pid(0,30511,29),memory).
{memory,20119888}

20 megabytes in small websocket =)

And I cannot inspect it with sys protocol.

If you are going to merge pull request, I can add support for sys protocol to websockets.

Owner

essen commented Feb 27, 2013

What I ultimately want is to have them be special processes, but not just websockets, all HTTP processes.

Contributor

maxlapshin commented Feb 27, 2013

but they already are special processes.

They just don't conform to sys protocol and thus I cannot introspect them inside.

On Feb 28, 2013, at 12:54 AM, Loïc Hoguin wrote:

What I ultimately want is to have them be special processes, but not just websockets, all HTTP processes.


Reply to this email directly or view it on GitHub.

Owner

essen commented Feb 27, 2013

Special processes mean started with proc_lib and handling sys messages, they do neither of these things at this time.

If they are made special processes, that means we need to handle sys messages everywhere, not just in the websocket code. You could do it just in the websocket code to debug your issue but that would not be enough for a merge (though a PR is still welcome).

Contributor

maxlapshin commented Feb 27, 2013

No, it is not interesting to write a code that will not be merged in upstream.

Ok, you are speaking about using proc_lib? It is great. But sys protocol is handled explicitly:
https://github.com/erlang/otp/blob/maint/lib/stdlib/src/gen_server.erl#L375

On Feb 28, 2013, at 12:58 AM, Loïc Hoguin wrote:

Special processes mean started with proc_lib and handling sys messages, they do neither of these things at this time.

If they are made special processes, that means we need to handle sys messages everywhere, not just in the websocket code. You could do it just in the websocket code to debug your issue but that would not be enough for a merge (though a PR is still welcome).


Reply to this email directly or view it on GitHub.

Owner

essen commented Feb 27, 2013

Yeah it would be a hackish way to make half of sys work, but then the other half would break horribly because it assumes the process was started using proc_lib. If you receive a system message saying the process should be suspended then your process will simply crash. We can't have that. Gotta go all the way.

Contributor

maxlapshin commented Feb 27, 2013

and what is the problem in starting with proc_lib?

It is widely used in erlyvideo without any problems: https://github.com/erlyvideo/flussonic/blob/master/apps/rtmp/src/rtmp_socket.erl#L96

On Feb 28, 2013, at 1:15 AM, Loïc Hoguin wrote:

Yeah it would be a hackish way to make half of sys work, but then the other half would break horribly because it assumes the process was started using proc_lib. If you receive a system message saying the process should be suspended then your process will simply crash. We can't have that. Gotta go all the way.


Reply to this email directly or view it on GitHub.

Owner

essen commented Feb 27, 2013

No problem? You just have to handle system messages everywhere and not just with websockets like I said.

Contributor

asabil commented Mar 1, 2013

You might want to take a look at gen_process: https://github.com/asabil/gen_process

Owner

essen commented Mar 1, 2013

It's doing much more than we want. We just need the proc_lib:start and handling system messages. The "hard" part is to find the right way to do it while still keeping the code as efficient as it is now.

Contributor

asabil commented Mar 2, 2013

Yes probably, which features do you see as being unnecessary in your case?

Owner

essen commented Mar 2, 2013

Pretty much all of it. It doesn't work quite the same way here, the process goes through many different modules, a good number of them needing to receive messages and handle system events. There's no need for calling the process either.

@essen essen modified the milestone: 2.0.0 Aug 18, 2015

@essen essen modified the milestone: 2.0.0 Feb 3, 2017

Owner

essen commented Feb 4, 2017

This is already in master. Closing, thanks!

@essen essen closed this Feb 4, 2017

Owner

essen commented Feb 4, 2017

Actually let me reopen because I think this is not finished. I will check again later. Sorry for the noise!

@essen essen reopened this Feb 4, 2017

@essen essen added this to the 2.0 Tasks milestone Feb 4, 2017

@essen essen removed this from the 2.0 Tasks milestone Oct 2, 2017

@essen essen added this to the 2.3 "Finish sys support" milestone Dec 12, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment