[thrift] gut Erlang exception handling
Summary: * move type field to tException from subclasses
* add backtrace to tException
* add oop:is_a
* on exit, wrap exceptions in {thrift_exception, E} ... otherwise can't distinguish e.g. exit:{{tBinProtException, {tException, ...}}, Stack} vs. exit:{tBinProtException, {tException, ...} -- I hate erlang
* all throws/exits to tException:throw which does the wrapping described above
Reviewed By: eletuchy
Test Plan: been using this code on my live server ^_^
Revert: OK
git-svn-id: https://svn.apache.org/repos/asf/incubator/thrift/trunk@665350 13f79535-47bb-0310-9956-ffa450edef68
diff --git a/lib/erl/src/transport/tErlAcceptor.erl b/lib/erl/src/transport/tErlAcceptor.erl
index 8398924..bcfcb9a 100644
--- a/lib/erl/src/transport/tErlAcceptor.erl
+++ b/lib/erl/src/transport/tErlAcceptor.erl
@@ -10,6 +10,7 @@
-include("thrift.hrl").
-include("tApplicationException.hrl").
-include("transport/tTransportException.hrl").
+-include("protocol/tProtocolException.hrl").
-include("transport/tServerSocket.hrl").
-include("transport/tErlAcceptor.hrl").
@@ -69,14 +70,13 @@
case catch gen_tcp:accept(ListenSocket) of
{ok, Socket} ->
- ?C0(ServerPid, effectful_new_acceptor), %% cast to create new acceptor
+ ?C0(ServerPid, effectful_new_acceptor), % cast to create new acceptor
AddrString = render_addr(Socket),
?INFO("thrift connection accepted from ~s", [AddrString]),
- %% start_new(tSocket, [])
Client = oop:start_new(tSocket, []),
- ?R1(Client, effectful_setHandle, Socket), %% TODO(cpiro): should we just let this be a param to the constructor?
+ ?R1(Client, effectful_setHandle, Socket),
%% cpiro: OPAQUE!! Trans = Client
TF = oop:get(This, transportFactory),
@@ -87,47 +87,66 @@
Prot = ?F1(PF, getProtocol, Trans),
%% start_new(, ...)
- Processor = oop:start_new(tErlProcessor, [GP, Handler]), %% TODO
+ Processor = oop:start_new(tErlProcessor, [GP, Handler]),
- case receive_loop(This, Processor, Prot, Prot) of
- conn_timeout ->
+ try
+ receive_loop(This, Processor, Prot, Prot)
+ catch
+ exit:{timeout, _} ->
?INFO("thrift connection timed out from ~s", [AddrString]);
- conn_closed ->
- ?INFO("thrift connection closed from ~s", [AddrString]);
- {Class, Else} ->
- ?ERROR("unhandled ~p in tErlAcceptor: ~p", [Class, Else])
+
+ %% cpiro: i think the extra entry on the stack is always from receive_loop
+ %% the below case shouldn't happen then? if we move this catch inside
+ %% we'll probably need this case and not the next one
+
+ %% exit:{thrift_exception, E} ->
+ %% handle_exception(E, AddrString, no2);
+
+ exit:{{thrift_exception, E}, Stack1} ->
+ handle_exception(E, AddrString, Stack1);
+
+ Class:Else ->
+ ?ERROR("some other error ~p in tErlAcceptor: ~p", [Class, Else])
end,
exit(normal);
Else ->
R = thrift_utils:sformat("accept() failed: ~p", [Else]),
- exit(tTransportException:new(R))
+ tException:throw(tTransportException, [R])
end.
+
+handle_exception(E, AddrString, Stack1) ->
+ case tException:read(E) of
+ none -> % not a tException
+ ?ERROR("not really a tException: ~p", [exit, E]);
+
+ {tProtocolException, ?tProtocolException_BAD_VERSION, _} ->
+ ?INFO("thrift missing version from ~s", [AddrString]);
+
+ {tTransportException, ?tTransportException_NOT_OPEN, _} ->
+ ?INFO("thrift connection closed from ~s", [AddrString]);
+
+ _ ->
+ Where = "thrift tErlAcceptor caught a tException",
+ ?ERROR("~s", [tException:inspect_with_backtrace(E, Where, Stack1)])
+ end.
+
+%% always calls itself ... only way to escape is through an exit
receive_loop(This, Processor, Iprot, Oprot) ->
- try ?R2(Processor, process, Iprot, Oprot) of
- {error, TAE} when is_record(TAE, tApplicationException),
- TAE#tApplicationException.type == ?tApplicationException_HANDLER_ERROR ->
- ?ERROR("thrift handler returned an error: ~p", [oop:get(TAE, message)]),
+ case ?R2(Processor, process, Iprot, Oprot) of
+ {error, Reason} ->
+ case tException:read(Reason) of
+ none ->
+ ?ERROR("thrift handler returned something weird: {error, ~p}", [Reason]);
+ _ ->
+ Where = "thrift processor/handler caught a tException",
+ ?ERROR("~s", [tException:inspect_with_backtrace(Reason, Where)])
+ end,
receive_loop(This, Processor, Iprot, Oprot);
Value ->
?INFO("thrift request: ~p", [Value]),
receive_loop(This, Processor, Iprot, Oprot)
- catch
- exit:{timeout, _} ->
- conn_timeout;
-
- %% the following clause must be last
- %% cpiro: would be best to implement an is_a/2 guard BIF
- %% cpiro: breaks if it's a subclass of tTransportException
- %% since unnest_record knows nothing about oop
- Class:Else ->
- case thrift_utils:unnest_record(Else, tTransportException) of
- {ok, TTE} when TTE#tTransportException.type == ?tTransportException_NOT_OPEN ->
- conn_closed;
- _ ->
- {Class, Else}
- end
end.
%% helper functions
diff --git a/lib/erl/src/transport/tSocket.erl b/lib/erl/src/transport/tSocket.erl
index f002fd8..5aead1e 100644
--- a/lib/erl/src/transport/tSocket.erl
+++ b/lib/erl/src/transport/tSocket.erl
@@ -75,10 +75,8 @@
case gen_tcp:connect(Host, Port, Options) of
{error, _} ->
- exit(tTransportException:new(
- ?tTransportException_NOT_OPEN,
- "Could not connect to " ++ Host ++ ":" ++ Port)
- );
+ tException:throw(tTransportException,
+ [?tTransportException_NOT_OPEN, "Could not connect to " ++ Host ++ ":" ++ Port]);
{ok, Socket} ->
effectful_setHandle(This, Socket)
end.
@@ -97,7 +95,7 @@
case Val of
{error, _} ->
- throw(tTransportException:new(?tTransportException_NOT_OPEN, "in write"));
+ tException:throw(tTransportException, [?tTransportException_NOT_OPEN, "in write"]);
ok ->
{ok, This}
end.
@@ -108,13 +106,13 @@
{ok, []} ->
Host = oop:get(This, host),
Port = oop:get(This, port),
- throw(tTransportException:new(?tTransportException_UNKNOWN, "TSocket: Could not read " ++ Sz ++ "bytes from " ++ Host ++ ":" ++ Port));
+ tException:throw(tTransportException, [?tTransportException_UNKNOWN, "TSocket: Could not read " ++ Sz ++ "bytes from " ++ Host ++ ":" ++ Port]);
{ok, Data} ->
%% DEBUG
?INFO("tSocket: read ~p", [Data]),
Data;
{error, Error} ->
- exit(tTransportException:new(?tTransportException_NOT_OPEN, "in tSocket:read/2: gen_tcp:recv"))
+ tException:throw(tTransportException, [?tTransportException_NOT_OPEN, "in tSocket:read/2: gen_tcp:recv"])
end.
effectful_close(This) ->
diff --git a/lib/erl/src/transport/tTransportException.erl b/lib/erl/src/transport/tTransportException.erl
index b7dc43d..f81ae9b 100644
--- a/lib/erl/src/transport/tTransportException.erl
+++ b/lib/erl/src/transport/tTransportException.erl
@@ -22,8 +22,7 @@
%%% 'super' is required unless ?MODULE is a base class
%%%
-?DEFINE_ATTR(super);
-?DEFINE_ATTR(type).
+?DEFINE_ATTR(super).
%%%
%%% behavior callbacks
@@ -38,15 +37,15 @@
%%% inspect(This) -> string()
inspect(This) ->
- ?FORMAT_ATTR(type).
+ "".
%%%
%%% class methods
%%%
new(Type, Message) ->
- Super = (super()):new(Message),
- #?MODULE{super=Super, type=Type}.
+ Super = (super()):new(Type, Message),
+ #?MODULE{super=Super}.
new() ->
new(?tTransportException_UNKNOWN, undefined).