Separate message and packet handling for websockets

Improves the readability of websocket handler code by having
two functions: websocket_handle/3 handles the packets received
from the socket, removing the tuple construct that was otherwise
needed, so only websocket_handle(Data, Req, State) is needed now;
websocket_info/3 handles the messages that the websocket handler
process received, as websocket_info(Info, Req, State).

Both functions return values are handled identically by Cowboy
so nothing changes on that end.

README.md

@@ -179,7 +179,8 @@ Websocket would look like this:
 -behaviour(cowboy_http_handler).
 -behaviour(cowboy_http_websocket_handler).
 -export([init/3, handle/2, terminate/2]).
--export([websocket_init/3, websocket_handle/3, websocket_terminate/3]).
+-export([websocket_init/3, websocket_handle/3,
+    websocket_info/3, websocket_terminate/3]).
 
 init({tcp, http}, Req, Opts) ->
     {upgrade, protocol, cowboy_http_websocket}.
@@ -194,11 +195,14 @@ websocket_init(TransportName, Req, _Opts) ->
     erlang:start_timer(1000, self(), <<"Hello!">>),
     {ok, Req, undefined_state}.
 
-websocket_handle({timeout, _Ref, Msg}, Req, State) ->
+websocket_handle(Msg, Req, State) ->
+    {reply, << "That's what she said! ", Msg/binary >>, Req, State}.
+
+websocket_info({timeout, _Ref, Msg}, Req, State) ->
     erlang:start_timer(1000, self(), <<"How' you doin'?">>),
     {reply, Msg, Req, State};
-websocket_handle({websocket, Msg}, Req, State) ->
+websocket_info(_Info, Req, State) ->
-    {reply, << "That's what she said! ", Msg/binary >>, Req, State}.
+    {ok, Req, State}.
 
 websocket_terminate(_Reason, _Req, _State) ->
     ok.

src/cowboy_http_websocket.erl

@@ -157,7 +157,7 @@ handler_loop(State=#state{messages={OK, Closed, Error}, timeout=Timeout},
 			handler_terminate(State, Req, HandlerState, {error, Reason});
 		Message ->
 			handler_call(State, Req, HandlerState,
-				SoFar, Message, fun handler_before_loop/4)
+				SoFar, websocket_info, Message, fun handler_before_loop/4)
 	after Timeout ->
 		websocket_close(State, Req, HandlerState, {normal, timeout})
 	end.
@@ -178,7 +178,7 @@ websocket_frame(State=#state{eop=EOP}, Req, HandlerState, Data, 0) ->
 			Pos2 = Pos - 1,
 			<< 0, Frame:Pos2/binary, 255, Rest/bits >> = Data,
 			handler_call(State, Req, HandlerState,
-				Rest, {websocket, Frame}, fun websocket_data/4);
+				Rest, websocket_handle, Frame, fun websocket_data/4);
 		nomatch ->
 			%% @todo We probably should allow limiting frame length.
 			handler_before_loop(State, Req, HandlerState, Data)
@@ -186,10 +186,11 @@ websocket_frame(State=#state{eop=EOP}, Req, HandlerState, Data, 0) ->
 websocket_frame(State, Req, HandlerState, _Data, _FrameType) ->
 	websocket_close(State, Req, HandlerState, {error, badframe}).
 
--spec handler_call(#state{}, #http_req{}, any(), binary(), any(), fun()) -> ok.
+-spec handler_call(#state{}, #http_req{}, any(), binary(),
+	atom(), any(), fun()) -> ok.
 handler_call(State=#state{handler=Handler, opts=Opts}, Req, HandlerState,
-		RemainingData, Message, NextState) ->
+		RemainingData, Callback, Message, NextState) ->
-	try Handler:websocket_handle(Message, Req, HandlerState) of
+	try Handler:Callback(Message, Req, HandlerState) of
 		{ok, Req2, HandlerState2} ->
 			NextState(State, Req2, HandlerState2, RemainingData);
 		{ok, Req2, HandlerState2, hibernate} ->
@@ -242,16 +243,19 @@ handler_terminate(#state{handler=Handler, opts=Opts},
 			 HandlerState, Req, erlang:get_stacktrace()])
 	end.
 
-%% eunit
+%% Tests.
 
 -ifdef(TEST).
 
 websocket_location_test() ->
-  ?assertEqual(<<"ws://localhost/path">>, websocket_location(other, <<"localhost">>, 80, <<"/path">>)),
+	?assertEqual(<<"ws://localhost/path">>,
-  ?assertEqual(<<"ws://localhost:8080/path">>, websocket_location(other, <<"localhost">>, 8080, <<"/path">>)),
+		websocket_location(other, <<"localhost">>, 80, <<"/path">>)),
-  ?assertEqual(<<"wss://localhost/path">>, websocket_location(ssl, <<"localhost">>, 443, <<"/path">>)),
+	?assertEqual(<<"ws://localhost:8080/path">>,
-  ?assertEqual(<<"wss://localhost:8443/path">>, websocket_location(ssl, <<"localhost">>, 8443, <<"/path">>)),
+		websocket_location(other, <<"localhost">>, 8080, <<"/path">>)),
-  ok.
+	?assertEqual(<<"wss://localhost/path">>,
+		websocket_location(ssl, <<"localhost">>, 443, <<"/path">>)),
+	?assertEqual(<<"wss://localhost:8443/path">>,
+		websocket_location(ssl, <<"localhost">>, 8443, <<"/path">>)),
+	ok.
 
 -endif.
-

src/cowboy_http_websocket_handler.erl

@@ -14,12 +14,13 @@
 
 %% @doc Handler for HTTP WebSocket requests.
 %%
-%% WebSocket handlers must implement three callbacks: <em>websocket_init/3</em>,
+%% WebSocket handlers must implement four callbacks: <em>websocket_init/3</em>,
-%% <em>websocket_handle/3</em> and <em>websocket_terminate/3</em>. These
+%% <em>websocket_handle/3</em>, <em>websocket_info/3</em> and
-%% callbacks will only be called if the connection is upgraded to WebSocket
+%% <em>websocket_terminate/3</em>. These callbacks will only be called if the
-%% in the HTTP handler's <em>init/3</em> callback. They are then called in that
+%% connection is upgraded to WebSocket in the HTTP handler's <em>init/3</em>
-%% order, although <em>websocket_handle/3</em> will be called multiple time,
+%% callback. They are then called in that order, although
-%% one time for each message or packet received.
+%% <em>websocket_handle/3</em> will be called for each packet received,
+%% and <em>websocket_info</em> for each message received.
 %%
 %% <em>websocket_init/3</em> is meant for initialization. It receives
 %% information about the transport and protocol used, along with the handler
@@ -27,11 +28,15 @@
 %% If you are going to want to compact the request, you should probably do it
 %% here.
 %%
-%% <em>websocket_handle/3</em> receives messages sent to the process and
+%% <em>websocket_handle/3</em> receives the data from the socket. It can reply
-%% also the data sent to the socket. In the later case the information is
+%% something, do nothing or close the connection. You can choose to hibernate
-%% given as a tuple <em>{websocket, Data}</em>. It can reply something, do
+%% the process by returning <em>hibernate</em> to save memory and CPU.
-%% nothing or close the connection. You can choose to hibernate the process
+%%
-%% by returning <em>hibernate</em> to save memory and CPU.
+%% <em>websocket_info/3</em> receives messages sent to the process. It has
+%% the same reply format as <em>websocket_handle/3</em> described above. Note
+%% that unlike in a <em>gen_server</em>, when <em>websocket_info/3</em>
+%% replies something, it is always to the socket, not to the process that
+%% originated the message.
 %%
 %% <em>websocket_terminate/3</em> is meant for cleaning up. It also receives
 %% the request and the state previously defined, along with a reason for
@@ -41,9 +46,11 @@
 -export([behaviour_info/1]).
 
 %% @private
--spec behaviour_info(_) -> undefined | [{websocket_handle, 3}
+-spec behaviour_info(_)
-	| {websocket_init, 3} | {websocket_terminate, 3}, ...].
+	-> undefined | [{websocket_handle, 3} | {websocket_info, 3}
+		| {websocket_init, 3} | {websocket_terminate, 3}, ...].
 behaviour_info(callbacks) ->
-	[{websocket_init, 3}, {websocket_handle, 3}, {websocket_terminate, 3}];
+	[{websocket_init, 3}, {websocket_handle, 3},
+	 {websocket_info, 3}, {websocket_terminate, 3}];
 behaviour_info(_Other) ->
 	undefined.

test/websocket_handler.erl

@@ -4,7 +4,8 @@
 -behaviour(cowboy_http_handler).
 -behaviour(cowboy_http_websocket_handler).
 -export([init/3, handle/2, terminate/2]).
--export([websocket_init/3, websocket_handle/3, websocket_terminate/3]).
+-export([websocket_init/3, websocket_handle/3,
+	websocket_info/3, websocket_terminate/3]).
 
 init(_Any, _Req, _Opts) ->
 	{upgrade, protocol, cowboy_http_websocket}.
@@ -19,11 +20,14 @@ websocket_init(_TransportName, Req, _Opts) ->
 	erlang:start_timer(1000, self(), <<"websocket_init">>),
 	{ok, Req, undefined}.
 
-websocket_handle({timeout, _Ref, Msg}, Req, State) ->
+websocket_handle(Data, Req, State) ->
+	{reply, Data, Req, State}.
+
+websocket_info({timeout, _Ref, Msg}, Req, State) ->
 	erlang:start_timer(1000, self(), <<"websocket_handle">>),
 	{reply, Msg, Req, State};
-websocket_handle({websocket, Data}, Req, State) ->
+websocket_info(_Info, Req, State) ->
-	{reply, Data, Req, State}.
+	{ok, Req, State}.
 
 websocket_terminate(_Reason, _Req, _State) ->
 	ok.