From: Christopher Piro Date: Sun, 18 Nov 2007 02:10:20 +0000 (+0000) Subject: [thrift] gut Erlang exception handling X-Git-Tag: 0.2.0~1122 X-Git-Url: https://source.supwisdom.com/gerrit/gitweb?a=commitdiff_plain;h=de11d852113dcb2284c54ab47333e6f602320fc9;p=common%2Fthrift.git [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/compiler/cpp/src/generate/t_erl_generator.cc b/compiler/cpp/src/generate/t_erl_generator.cc index d152c182..de807a7c 100644 --- a/compiler/cpp/src/generate/t_erl_generator.cc +++ b/compiler/cpp/src/generate/t_erl_generator.cc @@ -717,7 +717,6 @@ void t_erl_generator::generate_service_client(t_service* tservice) { << "=" << capitalize((*fld_iter)->get_name()); } f_service_ << "}," << endl; - indent(f_service_) << (*f_iter)->get_name() << "_args_write(Args, Oprot)," << endl; // Write to the stream diff --git a/lib/erl/include/oop.hrl b/lib/erl/include/oop.hrl index 84d81e1b..66d1a0c8 100644 --- a/lib/erl/include/oop.hrl +++ b/lib/erl/include/oop.hrl @@ -1,6 +1,6 @@ %%% Copyright (c) 2007- Facebook %%% Distributed under the Thrift Software License -%%% +%%% %%% See accompanying file LICENSE or visit the Thrift site at: %%% http://developers.facebook.com/thrift/ @@ -16,10 +16,10 @@ %%% convenience for implementing inspect/1 %%% e.g. -> "foo=5" -define(FORMAT_ATTR(Attr), - io_lib:write_atom(Attr) ++ "=" ++ io_lib:print(?ATTR(Attr)) + io_lib:write_atom(Attr) ++ "=" ++ io_lib:print(?ATTR(Attr)) ). --define(ATTR_DUMMY, - attr(dummy, dummy, dummy, dummy) -> - throw(dummy_attr_used) +-define(ATTR_DUMMY, + attr(dummy, dummy, dummy, dummy) -> + exit(dummy_attr_used) ). diff --git a/lib/erl/include/protocol/tProtocolException.hrl b/lib/erl/include/protocol/tProtocolException.hrl index 2de72d71..9d2f31af 100644 --- a/lib/erl/include/protocol/tProtocolException.hrl +++ b/lib/erl/include/protocol/tProtocolException.hrl @@ -1,6 +1,6 @@ %%% Copyright (c) 2007- Facebook %%% Distributed under the Thrift Software License -%%% +%%% %%% See accompanying file LICENSE or visit the Thrift site at: %%% http://developers.facebook.com/thrift/ @@ -10,6 +10,6 @@ -define(tProtocolException_SIZE_LIMIT, 3). -define(tProtocolException_BAD_VERSION, 4). --record(tProtocolException, {super, type}). +-record(tProtocolException, {super}). diff --git a/lib/erl/include/tApplicationException.hrl b/lib/erl/include/tApplicationException.hrl index db7ec2ff..5e2b515c 100644 --- a/lib/erl/include/tApplicationException.hrl +++ b/lib/erl/include/tApplicationException.hrl @@ -1,6 +1,6 @@ %%% Copyright (c) 2007- Facebook %%% Distributed under the Thrift Software License -%%% +%%% %%% See accompanying file LICENSE or visit the Thrift site at: %%% http://developers.facebook.com/thrift/ @@ -13,4 +13,4 @@ -define(tApplicationException_MISSING_RESULT, 5). -define(tApplicationException_HANDLER_ERROR, 6). --record(tApplicationException, {super, type}). +-record(tApplicationException, {super}). diff --git a/lib/erl/include/tException.hrl b/lib/erl/include/tException.hrl index 808a4749..896e8cb3 100644 --- a/lib/erl/include/tException.hrl +++ b/lib/erl/include/tException.hrl @@ -1,7 +1,7 @@ %%% Copyright (c) 2007- Facebook %%% Distributed under the Thrift Software License -%%% +%%% %%% See accompanying file LICENSE or visit the Thrift site at: %%% http://developers.facebook.com/thrift/ --record(tException, {message}). +-record(tException, {message, type, backtrace}). diff --git a/lib/erl/include/thrift_constants.hrl b/lib/erl/include/thrift_constants.hrl index 8aad0a92..19480610 100644 --- a/lib/erl/include/thrift_constants.hrl +++ b/lib/erl/include/thrift_constants.hrl @@ -1,6 +1,6 @@ %%% Copyright (c) 2007- Facebook %%% Distributed under the Thrift Software License -%%% +%%% %%% See accompanying file LICENSE or visit the Thrift site at: %%% http://developers.facebook.com/thrift/ diff --git a/lib/erl/include/thrift_macros.hrl b/lib/erl/include/thrift_macros.hrl index 9b8a2008..a1a6bad0 100644 --- a/lib/erl/include/thrift_macros.hrl +++ b/lib/erl/include/thrift_macros.hrl @@ -1,6 +1,6 @@ %%% Copyright (c) 2007- Facebook %%% Distributed under the Thrift Software License -%%% +%%% %%% See accompanying file LICENSE or visit the Thrift site at: %%% http://developers.facebook.com/thrift/ diff --git a/lib/erl/include/transport/tTransportException.hrl b/lib/erl/include/transport/tTransportException.hrl index 1a60aad1..0fcc99fc 100644 --- a/lib/erl/include/transport/tTransportException.hrl +++ b/lib/erl/include/transport/tTransportException.hrl @@ -1,6 +1,6 @@ %%% Copyright (c) 2007- Facebook %%% Distributed under the Thrift Software License -%%% +%%% %%% See accompanying file LICENSE or visit the Thrift site at: %%% http://developers.facebook.com/thrift/ @@ -10,4 +10,4 @@ -define(tTransportException_TIMED_OUT, 3). -define(tTransportException_END_OF_FILE, 4). --record(tTransportException, {super, type}). +-record(tTransportException, {super}). diff --git a/lib/erl/src/oop.erl b/lib/erl/src/oop.erl index e4de147a..418f0f28 100644 --- a/lib/erl/src/oop.erl +++ b/lib/erl/src/oop.erl @@ -14,7 +14,7 @@ -module(oop). --export([start_new/2, get/2, set/3, call/2, call/3, inspect/1, class/1, is_object/1]). +-export([start_new/2, get/2, set/3, call/2, call/3, inspect/1, class/1, is_object/1, is_a/2]). -export([call1/3]). %% only for thrift_oop_server ... don't use it -export([behaviour_info/1]). @@ -65,7 +65,7 @@ set(Obj, Field, Value) -> %% TODO: could be tail-recursive {ok, V} -> V; undef -> case get_superobject(Obj) of - { ok, Superobj } -> + {ok, Superobj} -> Superobj1 = set(Superobj, Field, Value), Module:attr(Obj, set, super, Superobj1); undef -> @@ -180,11 +180,30 @@ inspect1(Obj, Str) -> class(Obj) when is_tuple(Obj) -> %% if it's an object its first element will be a class name, and it'll have super/0 case apply_if_defined(?CLASS(Obj), super, []) of - {ok, V} -> V; + {ok, _} -> ?CLASS(Obj); undef -> none end; class(_) -> none. +%% is_a relationship +is_a(Obj, Class) -> + %% ?INFO("is_a ~p ~p", [Obj, Class]), + case is_object(Obj) of + true -> + is_a1(Obj, Class); + false -> + false + end. +is_a1(Obj, Class) when Class == ?CLASS(Obj) -> + true; +is_a1(Obj, Class) -> + case get_superobject(Obj) of + undef -> + false; + {ok, SuperObj} -> + is_a1(SuperObj, Class) + end. + %% is the tuple/record an object? %% is_object(Obj) = bool() is_object(Obj) when is_tuple(Obj) -> @@ -209,7 +228,7 @@ apply_if_defined({M,F,A} = MFA) -> %% io:format("apply ~p ~p ~p~n", [M,F,A]), {ok, apply(M, F, A)} catch - error:Kind when Kind == undef; Kind == function_clause -> + _:Kind when Kind == undef; Kind == function_clause -> case erlang:get_stacktrace() of %% the first stack call should match MFA when `apply' fails because the function is undefined %% they won't match if the function is currently running and an error happens in the middle diff --git a/lib/erl/src/protocol/tBinaryProtocol.erl b/lib/erl/src/protocol/tBinaryProtocol.erl index 207f305f..efcfd779 100644 --- a/lib/erl/src/protocol/tBinaryProtocol.erl +++ b/lib/erl/src/protocol/tBinaryProtocol.erl @@ -136,14 +136,13 @@ writeString(This, Str) -> ?L1(writeI32, length(Str)), ?R1(Trans, effectful_write, Str). -% +%% readMessageBegin(This) -> Version = ?L0(readI32), if (Version band ?VERSION_MASK) /= ?VERSION_1 -> - throw(tProtocolException:new(?tProtocolException_BAD_VERSION, - "Missing version identifier")); + tException:throw(tProtocolException, [?tProtocolException_BAD_VERSION, "Missing version identifier"]); true -> ok end, Type = Version band 16#000000ff, @@ -177,7 +176,7 @@ readSetBegin(This) -> Size = ?L0(readI32), { Etype, Size }. -% +%% readBool(This) -> Byte = ?L0(readByte), diff --git a/lib/erl/src/protocol/tProtocolException.erl b/lib/erl/src/protocol/tProtocolException.erl index d926aff3..84833a82 100644 --- a/lib/erl/src/protocol/tProtocolException.erl +++ b/lib/erl/src/protocol/tProtocolException.erl @@ -1,6 +1,6 @@ %%% Copyright (c) 2007- Facebook %%% Distributed under the Thrift Software License -%%% +%%% %%% See accompanying file LICENSE or visit the Thrift site at: %%% http://developers.facebook.com/thrift/ @@ -22,13 +22,12 @@ %%% 'super' is required unless ?MODULE is a base class %%% -?DEFINE_ATTR(super); -?DEFINE_ATTR(type). - +?DEFINE_ATTR(super). + %%% %%% behavior callbacks %%% - + %%% super() -> SuperModule = atom() %%% | none @@ -38,15 +37,15 @@ super() -> %%% 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(?tProtocolException_UNKNOWN, undefined). diff --git a/lib/erl/src/server/tErlServer.erl b/lib/erl/src/server/tErlServer.erl index 4e1f6fc5..10ac2b25 100644 --- a/lib/erl/src/server/tErlServer.erl +++ b/lib/erl/src/server/tErlServer.erl @@ -1,6 +1,6 @@ %%% Copyright (c) 2007- Facebook %%% Distributed under the Thrift Software License -%%% +%%% %%% See accompanying file LICENSE or visit the Thrift site at: %%% http://developers.facebook.com/thrift/ @@ -16,7 +16,7 @@ -export([attr/4, super/0, inspect/1]). --export([new/6, new/5, new/4, effectful_serve/1, effectful_new_acceptor/1, catches/3]). +-export([new/6, new/5, new/4, effectful_serve/1, effectful_new_acceptor/1]). %%% %%% define attributes @@ -27,11 +27,11 @@ ?DEFINE_ATTR(acceptor); ?DEFINE_ATTR(listenSocket); ?DEFINE_ATTR(port). - + %%% %%% behavior callbacks %%% - + %%% super() -> SuperModule = atom() %%% | none @@ -67,20 +67,20 @@ effectful_serve(This) -> Options = [binary, {packet, 0}, {active, false}], %% listen - case gen_tcp:listen(Port, Options) of - {ok, ListenSocket} -> - ?INFO("thrift server listening on port ~p", [Port]), + case gen_tcp:listen(Port, Options) of + {ok, ListenSocket} -> + ?INFO("thrift server listening on port ~p", [Port]), - This1 = oop:set(This, listenSocket, ListenSocket), + This1 = oop:set(This, listenSocket, ListenSocket), - %% spawn acceptor - {_Acceptor, This2} = effectful_new_acceptor(This1), + %% spawn acceptor + {_Acceptor, This2} = effectful_new_acceptor(This1), - {ok, This2}; + {ok, This2}; - {error, eaddrinuse} -> - ?ERROR("thrift couldn't bind port ~p, address in use", [Port]), - {{error, eaddrinuse}, This} %% state before the accept + {error, eaddrinuse} -> + ?ERROR("thrift couldn't bind port ~p, address in use", [Port]), + {{error, eaddrinuse}, This} %% state before the accept end. effectful_new_acceptor(This) -> @@ -100,18 +100,3 @@ effectful_new_acceptor(This) -> This1 = oop:set(This, acceptor, Acceptor), {Acceptor, This1}. - -catches(_This, _Pid, normal) -> - ok. - -%% %% The current acceptor has died, wait a little and try again %% -%% handle_info({'EXIT', Pid, _Abnormal}, #state{acceptor=Pid} = State) -> %% -%% timer:sleep(2000), %% -%% iserve_socket:start_link(self(), State#state.listen_socket, State#state.port), %% -%% {noreply,State}; %% - -%% terminate(Reason, State) -> %% -%% ?INFO( "Terminating error: ~p~n", [Reason]), % added %% -%% gen_tcp:close(State#state.listen_socket), %% -%% ok. %% -%% %% diff --git a/lib/erl/src/tApplicationException.erl b/lib/erl/src/tApplicationException.erl index 568b9c94..d99b0037 100644 --- a/lib/erl/src/tApplicationException.erl +++ b/lib/erl/src/tApplicationException.erl @@ -1,6 +1,6 @@ %%% Copyright (c) 2007- Facebook %%% Distributed under the Thrift Software License -%%% +%%% %%% See accompanying file LICENSE or visit the Thrift site at: %%% http://developers.facebook.com/thrift/ @@ -22,9 +22,8 @@ %%% 'super' is required unless ?MODULE is a base class %%% -?DEFINE_ATTR(super); -?DEFINE_ATTR(type). - +?DEFINE_ATTR(super). + %%% %%% behavior callbacks %%% @@ -38,15 +37,15 @@ super() -> %%% 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(?tApplicationException_UNKNOWN, undefined). new(Type) -> new(Type, undefined). @@ -64,49 +63,49 @@ read(This, Iprot) -> read_while_loop(This, Iprot) -> {_Fname, Ftype, Fid} = ?R0(Iprot, readFieldBegin), - if - Ftype == ?tType_STOP -> - ok; - - (Fid == 1) and (Ftype == ?tType_STRING) -> - Message1 = ?R0(Iprot, readString), - This1 = oop:set(This, message, Message1), - ?R0(Iprot, readFieldEnd), - read_while_loop(This1, Iprot); - - Fid == 1 -> - ?R0(Iprot, skip), - ?R0(Iprot, readFieldEnd), - read_while_loop(This, Iprot); - - (Fid == 2) and (Ftype == ?tType_I32) -> - Type1 = ?R0(Iprot, readI32), - This1 = oop:set(This, type, Type1), - ?R0(Iprot, readFieldEnd), - read_while_loop(This1, Iprot); - - true -> - ?R0(Iprot, skip), - ?R0(Iprot, readFieldEnd), - read_while_loop(This, Iprot) + if + Ftype == ?tType_STOP -> + ok; + + (Fid == 1) and (Ftype == ?tType_STRING) -> + Message1 = ?R0(Iprot, readString), + This1 = oop:set(This, message, Message1), + ?R0(Iprot, readFieldEnd), + read_while_loop(This1, Iprot); + + Fid == 1 -> + ?R0(Iprot, skip), + ?R0(Iprot, readFieldEnd), + read_while_loop(This, Iprot); + + (Fid == 2) and (Ftype == ?tType_I32) -> + Type1 = ?R0(Iprot, readI32), + This1 = oop:set(This, type, Type1), + ?R0(Iprot, readFieldEnd), + read_while_loop(This1, Iprot); + + true -> + ?R0(Iprot, skip), + ?R0(Iprot, readFieldEnd), + read_while_loop(This, Iprot) end. -write(This, Oprot) -> +write(This, Oprot) -> ?R1(Oprot, writeStructBegin, "tApplicationException"), Message = oop:get(This, message), Type = oop:get(This, type), - if Message /= undefined -> - ?R3(Oprot, writeFieldBegin, "message", ?tType_STRING, 1), - ?R1(Oprot, writeString, Message), - ?R0(Oprot, writeFieldEnd); + if Message /= undefined -> + ?R3(Oprot, writeFieldBegin, "message", ?tType_STRING, 1), + ?R1(Oprot, writeString, Message), + ?R0(Oprot, writeFieldEnd); true -> ok end, - if Type /= undefined -> - ?R3(Oprot, writeFieldBegin, "type", ?tType_I32, 2), - ?R1(Oprot, writeI32, Type), - ?R0(Oprot, writeFieldEnd); + if Type /= undefined -> + ?R3(Oprot, writeFieldBegin, "type", ?tType_I32, 2), + ?R1(Oprot, writeI32, Type), + ?R0(Oprot, writeFieldEnd); true -> ok end, diff --git a/lib/erl/src/tErlProcessor.erl b/lib/erl/src/tErlProcessor.erl index a6d10738..2e88b6d3 100644 --- a/lib/erl/src/tErlProcessor.erl +++ b/lib/erl/src/tErlProcessor.erl @@ -1,6 +1,6 @@ %%% Copyright (c) 2007- Facebook %%% Distributed under the Thrift Software License -%%% +%%% %%% See accompanying file LICENSE or visit the Thrift site at: %%% http://developers.facebook.com/thrift/ @@ -24,11 +24,11 @@ ?DEFINE_ATTR(super); ?DEFINE_ATTR(generatedProcessor); ?DEFINE_ATTR(handler). - + %%% %%% behavior callbacks %%% - + %%% super() -> SuperModule = atom() %%% | none diff --git a/lib/erl/src/tException.erl b/lib/erl/src/tException.erl index 0ec4c948..6dd30844 100644 --- a/lib/erl/src/tException.erl +++ b/lib/erl/src/tException.erl @@ -1,31 +1,36 @@ %%% Copyright (c) 2007- Facebook %%% Distributed under the Thrift Software License -%%% +%%% %%% See accompanying file LICENSE or visit the Thrift site at: %%% http://developers.facebook.com/thrift/ -module(tException). -include("oop.hrl"). +-include("thrift.hrl"). -include("tException.hrl"). -behavior(oop). -export([attr/4, super/0, inspect/1]). --export([new/1]). +-export([new/2, add_backtrace_element/2, throw/2, inspect_with_backtrace/2, inspect_with_backtrace/3]). + +-export([read/1]). %%% %%% define attributes %%% 'super' is required unless ?MODULE is a base class %%% -?DEFINE_ATTR(message). - +?DEFINE_ATTR(message); +?DEFINE_ATTR(type); +?DEFINE_ATTR(backtrace). + %%% %%% behavior callbacks %%% - + %%% super() -> SuperModule = atom() %%% | none @@ -35,16 +40,54 @@ super() -> %%% inspect(This) -> string() inspect(This) -> - ?FORMAT_ATTR(message). + BT = ?ATTR(backtrace), + Depth = + if + is_list(BT) -> integer_to_list(length(BT)); + true -> "?" + end, + ?FORMAT_ATTR(message) ++ ", " ++ + ?FORMAT_ATTR(type) ++ ", " + " backtrace:" ++ Depth. %%% %%% class methods %%% -new(Message) -> - #?MODULE{message=Message}. +new(Type, Message) -> + #?MODULE{type=Type, message=Message, backtrace=[]}. -%%% -%%% instance methods -%%% +add_backtrace_element(E, Info) -> + BT = oop:get(E, backtrace), + E1 = oop:set(E, backtrace, [Info|BT]), + E1. + +throw(Class, Args) -> + E = apply(Class, new, Args), + exit({thrift_exception, E}). + + +inspect_with_backtrace(E, Where, Info) -> + E1 = add_backtrace_element(E, Info), + inspect_with_backtrace(E1, Where). + +inspect_with_backtrace(E, Where) -> + thrift_utils:sformat("** ~s~n** ~s", [Where, oop:inspect(E)]) ++ + case oop:get(E, backtrace) of + [] -> + ""; + BT when is_list(BT) -> + thrift_utils:sformat("~n** trace = ~p", [lists:reverse(BT)]); + Else -> + thrift_utils:sformat("", [Else]) + end. +read(E) -> + case oop:class(E) of + none -> + none; + Class -> + Type = oop:get(E, type), + BT = oop:get(E, backtrace), + {Class, Type, BT} + end. diff --git a/lib/erl/src/thrift_logger.erl b/lib/erl/src/thrift_logger.erl index 0d0b876c..c8fb49a0 100644 --- a/lib/erl/src/thrift_logger.erl +++ b/lib/erl/src/thrift_logger.erl @@ -8,14 +8,8 @@ -behaviour(gen_event). -%% TODO(cpiro): either -%% make exceptions know whether they need to be displayed -%% or not exit with tExecptions for non-errors -%% or "register" tExceptions with the logger (I LIKE!) -%% ... we shouldn't need to build any specifics in here -include("thrift.hrl"). -include("oop.hrl"). --include("transport/tTransportException.hrl"). %% gen_event callbacks -export([init/1, handle_event/2, handle_call/2, @@ -29,9 +23,10 @@ -define(GS_TERM_FORMAT, "** Generic server ~p terminating \n** Last message in was ~p~n** When Server state == ~p~n** Reason for termination == ~n** ~p~n"). -%% +%%% +%%% ensure the regular logger is out and ours is in +%%% -%% ensure the regular logger is out and ours is in install() -> %% remove loggers io:format("starting logger~n"), @@ -48,32 +43,18 @@ install() -> ok. -%%==================================================================== -%% gen_event callbacks -%%==================================================================== -%%-------------------------------------------------------------------- -%% @spec init(Args) -> {ok, State}. -%% -%% @doc -%% Whenever a new event handler is added to an event manager, -%% this function is called to initialize the event handler. -%% @end -%%-------------------------------------------------------------------- +%%% +%%% init +%%% + init([]) -> State = #state{}, {ok, State}. -%%-------------------------------------------------------------------- -%% @spec handle_event(Event, State) -> {ok, State} | -%% {swap_handler, Args1, State1, Mod2, Args2} | -%% remove_handler. -%% -%% @doc -%% Whenever an event manager receives an event sent using -%% gen_event:notify/2 or gen_event:sync_notify/2, this function is called for -%% each installed event handler to handle the event. -%% @end -%%-------------------------------------------------------------------- +%%% +%%% handle_event +%%% + handle_event2(Symbol, Pid, Type, Message, State) -> % Message must be a string {ok, MessageSafe, NL} = regexp:gsub(Message, "[\n]+", " "), % collapse whitespace to one space @@ -140,31 +121,14 @@ handle_event1({What, _Gleader, {Ref, Format, Data}}, State) when is_list(Format) end, case {Format, Data} of + {?GS_TERM_FORMAT, [Ref, LastMessage, Obj, {Kind, E}]} when Kind == timeout; Kind == thrift_exception -> + ok; + {?GS_TERM_FORMAT, [Ref, LastMessage, Obj, Reason]} -> - %% TODO: move as much logic as possible out of thrift_logger - Ignore = - begin - is_tuple(Reason) andalso - size(Reason) >= 1 andalso element(1, Reason) == timeout - end - orelse - begin - case thrift_utils:unnest_record(Reason, tTransportException) of - error -> false; - {ok, TTE} -> - oop:get(TTE, type) == ?tTransportException_NOT_OPEN andalso - oop:get(TTE, message) == "in tSocket:read/2: gen_tcp:recv" - end - end, - - case Ignore of - true -> - ok; - false -> - Format1 = "** gen_server terminating in message ~p~n** State = ~s~n** Reason = ~s~n", - Message = sformat(Format1, [LastMessage, oop:inspect(Obj), oop:inspect(Reason)]), %% TODO(cpiro): hope Reason is an object? - handle_event2(Symbol, Ref, "", Message, State) - end; + Format1 = "** gen_server terminating in message ~p~n** State = ~s~n** Reason = ~p~n", + Message = sformat(Format1, [LastMessage, oop:inspect(Obj), Reason]), + handle_event2(Symbol, Ref, "", Message, State); + {?GS_TERM_FORMAT, _Dta} -> Message = sformat("DATA DIDN'T MATCH: ~p~n", [Data]) ++ sformat(Format, Data), handle_event2(Symbol, Ref, "", Message, State); @@ -213,58 +177,23 @@ handle_event(Event, State) -> {ok, State} end. -%%-------------------------------------------------------------------- -%% @spec handle_call(Request, State) -> {ok, Reply, State} | -%% {swap_handler, Reply, Args1, State1, -%% Mod2, Args2} | -%% {remove_handler, Reply}. -%% -%% @doc -%% Whenever an event manager receives a request sent using -%% gen_event:call/3,4, this function is called for the specified event -%% handler to handle the request. -%% @end -%%-------------------------------------------------------------------- +%%% +%%% call, info, terminate, code_change +%%% + handle_call(_Request, State) -> Reply = ok, {ok, Reply, State}. -%%-------------------------------------------------------------------- -%% @spec handle_info(Info, State) -> {ok, State} | -%% {swap_handler, Args1, State1, Mod2, Args2} | -%% remove_handler. -%% -%% @doc -%% This function is called for each installed event handler when -%% an event manager receives any other message than an event or a synchronous -%% request (or a system message). -%% @end -%%-------------------------------------------------------------------- handle_info(_Info, State) -> {ok, State}. -%%-------------------------------------------------------------------- -%% @spec terminate(Reason, State) -> void(). -%% -%% @doc -%% Whenever an event handler is deleted from an event manager, -%% this function is called. It should be the opposite of Module:init/1 and -%% do any necessary cleaning up. -%% @end -%%-------------------------------------------------------------------- terminate(normal, _State) -> ok; terminate(Reason, _State) -> format("*****************~n~n frick, error logger terminating: ~p~n~n*****************~n~n", [Reason]), ok. -%%-------------------------------------------------------------------- -%% @spec code_change(OldVsn, State, Extra) -> {ok, NewState}. -%% -%% @doc -%% Convert process state when code is changed -%% @end -%%-------------------------------------------------------------------- code_change(_OldVsn, State, _Extra) -> {ok, State}. @@ -285,13 +214,10 @@ config(Item) -> print_crash_report(Report) -> case Report of - [[_,_,{error_info, XX}|_] | _] -> - case thrift_utils:first_item(XX) of - tTransportException -> - ok; - _ -> - io:format("~~~~ crash report: ~P~n", [XX, 3]) - end; + [[_,_,{error_info, {thrift_exception, _}}|_] | _] -> + ok; + [[_,_,{error_info, {timeout, _}}|_] | _] -> + ok; _ -> - io:format("~~~~ crash report (?): ~p~n", [Report]) + io:format("~~~~ crash report: ~w~n", [Report]) end. diff --git a/lib/erl/src/thrift_oop_server.erl b/lib/erl/src/thrift_oop_server.erl index 052578ce..d3bc7207 100644 --- a/lib/erl/src/thrift_oop_server.erl +++ b/lib/erl/src/thrift_oop_server.erl @@ -4,77 +4,43 @@ %%% See accompanying file LICENSE or visit the Thrift site at: %%% http://developers.facebook.com/thrift/ -%%%------------------------------------------------------------------- -%%% @doc -%%% @end -%%%------------------------------------------------------------------- -module(thrift_oop_server). -behaviour(gen_server). -%%-------------------------------------------------------------------- -%% Include files -%%-------------------------------------------------------------------- + -include("oop.hrl"). -include("thrift.hrl"). -%%-------------------------------------------------------------------- -%% External exports -%%-------------------------------------------------------------------- +-include("transport/tTransportException.hrl"). +-include("protocol/tProtocolException.hrl"). + -export([ start_link/0, stop/0 ]). -%%-------------------------------------------------------------------- -%% gen_server callbacks -%%-------------------------------------------------------------------- -export([init/1, handle_call/3, handle_cast/2, handle_info/2, terminate/2, code_change/3]). -%%-------------------------------------------------------------------- -%% record definitions -%%-------------------------------------------------------------------- - -%%-------------------------------------------------------------------- -%% macro definitions -%%-------------------------------------------------------------------- -define(SERVER, ?MODULE). -%%==================================================================== -%% External functions -%%==================================================================== -%%-------------------------------------------------------------------- -%% @doc Starts the server. -%% @spec start_link() -> {ok, pid()} | {error, Reason} -%% @end -%%-------------------------------------------------------------------- +%%% +%%% api +%%% + start_link() -> gen_server:start_link({local, ?SERVER}, ?MODULE, [], []). -%%-------------------------------------------------------------------- -%% @doc Stops the server. -%% @spec stop() -> ok -%% @end -%%-------------------------------------------------------------------- stop() -> gen_server:cast(?SERVER, stop). -%%==================================================================== -%% Server functions -%%==================================================================== - -%%-------------------------------------------------------------------- -%% Function: init/1 -%% Description: Initiates the server -%% Returns: {ok, State} | -%% {ok, State, Timeout} | -%% ignore | -%% {stop, Reason} -%%-------------------------------------------------------------------- +%%% +%%% init +%%% init({Class, Args}) -> process_flag(trap_exit, true), - try + try %% TODO use apply_if_defined State = apply(Class, new, Args), ?INFO("thrift ~p:new(~s) = ~s", [Class, thrift_utils:unbrack(Args), oop:inspect(State)]), {ok, State} @@ -85,132 +51,102 @@ init({Class, Args}) -> init(_) -> {stop, invalid_params}. -%%-------------------------------------------------------------------- -%% Function: handle_call/3 -%% Description: Handling call messages -%% Returns: {reply, Reply, State} | -%% {reply, Reply, State, Timeout} | -%% {noreply, State} | -%% {noreply, State, Timeout} | -%% {stop, Reason, Reply, State} | (terminate/2 is called) -%% {stop, Reason, State} (terminate/2 is called) -%%-------------------------------------------------------------------- - -handle_call(Request, From, State) -> - handle_either(call, Request, From, State). - -%%-------------------------------------------------------------------- -%% Function: handle_cast/2 -%% Description: Handling cast messages -%% Returns: {noreply, State} | -%% {noreply, State, Timeout} | -%% {stop, Reason, State} (terminate/2 is called) -%%-------------------------------------------------------------------- - -handle_cast(stop, State) -> - {stop, normal, State}; - -handle_cast({Method, Args}, State) -> - handle_either(cast, {Method, Args}, undefined, State). - --define(REPLY(Value, State), - case Type of - call -> {reply, Value, State}; - cast -> {noreply, State} - end -). - -handle_either(Type, Request, From, State) -> - %% error_logger:info_msg("~p: ~p", [?SERVER, oop:inspect(State)]), - %% error_logger:info_msg("handle_call(Request=~p, From=~p, State)", [Request, From]), +%%% +%%% call and cast +%%% + +handle_call(Request, From, State) -> handle_call_cast(call, Request, From, State). +handle_cast(stop, State) -> {stop, normal, State}; +handle_cast({Method, Args}, State) -> handle_call_cast(cast, {Method, Args}, undefined, State). + +reply(call, Value, State) -> {reply, Value, State}; +reply(cast, _Value, State) -> {noreply, State}. + +handle_call_cast(Type, Request, From, State) -> + %% ?INFO("~p: ~p", [?SERVER, oop:inspect(State)]), + %% ?INFO("handle_call(Request=~p, From=~p, State)", [Request, From]), case Request of {get, [Field]} -> Value = oop:get(State, Field), - ?REPLY(Value, State); + reply(Type, Value, State); {set, [Field, Value]} -> State1 = oop:set(State, Field, Value), - ?REPLY(Value, State1); + reply(Type, Value, State1); {class, []} -> - ?REPLY(?CLASS(State), State); + reply(Type, ?CLASS(State), State); {Method, Args} -> handle_method(Type, State, Method, Args); _ -> - error_logger:format("no match for Request = ~p", [Request]), - %% {stop, server_error, State} - {reply, server_error, State} + ?ERROR("thrift no match for Request = ~p", [Request]), + {stop, server_error, State} + %% {reply, server_error, State} end. handle_method(Type, State, Method, Args) -> - %% is an effectful call? Is_effectful = lists:prefix("effectful_", atom_to_list(Method)), - Call = oop:call(State, Method, Args), - %% TODO(cpiro): maybe add error handling here? = catch oop:call? - - case {Is_effectful, Call} of + try {Is_effectful, oop:call(State, Method, Args)} of {true, {Retval, State1}} -> - ?REPLY(Retval, State1); + reply(Type, Retval, State1); {true, _MalformedReturn} -> %% TODO(cpiro): bad match -- remove when we're done converting - error_logger:format("oop:call(effectful_*,..,..) malformed return value ~p", - [_MalformedReturn]), - %% {stop, server_error, State} - {noreply, State}; + ?ERROR("oop:call(effectful_*,..,..) malformed return value ~p", + [_MalformedReturn]), + {stop, server_error, State}; + %% {noreply, State}; {false, Retval} -> - ?REPLY(Retval, State) - end. + reply(Type, Retval, State) -%%-------------------------------------------------------------------- -%% Function: handle_info/2 -%% Description: Handling all non call/cast messages -%% Returns: {noreply, State} | -%% {noreply, State, Timeout} | -%% {stop, Reason, State} (terminate/2 is called) -%%-------------------------------------------------------------------- -handle_info({'EXIT', Pid, Except} = All, State) -> - Result = try - oop:call(State, catches, [Pid, Except]) catch - exit:{missing_method, _} -> - unhandled - end, + exit:{thrift_exception, E} -> handle_exception(E, nothing); + exit:{{thrift_exception, E}, Stack} -> handle_exception(E, Stack); + exit:normal -> exit(normal); + exit:(X = {timeout, _}) -> exit(X); + exit:Other -> + exit(Other) + end. + +handle_exception(E, Stack) -> + %% ?ERROR("texception ~p", [E]), + case {oop:is_a(E, tException), Stack} of + {true, nothing} -> % good + exit({thrift_exception, E}); + {true, _} -> % good + E1 = tException:add_backtrace_element(E, Stack), + exit({thrift_exception, E1}); + false -> % shit + ?ERROR("exception wasn't really a tException ~p", [E]), + exit(bum) + end. - case Result of - unhandled -> +%%% +%%% info, terminate, and code_change +%%% + +handle_info({'EXIT', Pid, Except} = All, State) -> + case Except of + normal -> + {noreply, State}; + {normal, _} -> + {noreply, State}; + _unhandled -> error_logger:format("unhandled exit ~p", [All]), - {stop, All, State}; - _WasHandled -> - {noreply, State} + {stop, All, State} end; handle_info(Info, State) -> - error_logger:info_msg("~p", [Info]), + ?INFO("~p", [Info]), {noreply, State}. -%%-------------------------------------------------------------------- -%% Function: terminate/2 -%% Description: Shutdown the server -%% Returns: any (ignored by gen_server) -%%-------------------------------------------------------------------- terminate(Reason, State) -> - %%error_logger:format("~p terminated!: ~p", [self(), Reason]), ok. -%%-------------------------------------------------------------------- -%% Func: code_change/3 -%% Purpose: Convert process state when code is changed -%% Returns: {ok, NewState} - %%-------------------------------------------------------------------- code_change(OldVsn, State, Extra) -> {ok, State}. - -%%==================================================================== -%%% Internal functions -%%==================================================================== diff --git a/lib/erl/src/thrift_sup.erl b/lib/erl/src/thrift_sup.erl index 8f6eaf73..ab3f80ef 100644 --- a/lib/erl/src/thrift_sup.erl +++ b/lib/erl/src/thrift_sup.erl @@ -1,6 +1,6 @@ %%% Copyright (c) 2007- Facebook %%% Distributed under the Thrift Software License -%%% +%%% %%% See accompanying file LICENSE or visit the Thrift site at: %%% http://developers.facebook.com/thrift/ @@ -29,7 +29,7 @@ init([Port, Handler, Processor]) -> Args = [SF, Port, Handler, Processor, ST, TF, PF], ThriftServer = {thrift_server, {?MODULE, thrift_start_link, Args}, - permanent, 2000, worker, ThriftModules}, + permanent, 2000, worker, ThriftModules}, {ok, {{one_for_one, 10, 1}, [ThriftServer]}}. diff --git a/lib/erl/src/thrift_utils.erl b/lib/erl/src/thrift_utils.erl index 8305224a..1cbacc02 100644 --- a/lib/erl/src/thrift_utils.erl +++ b/lib/erl/src/thrift_utils.erl @@ -1,6 +1,6 @@ %%% Copyright (c) 2007- Facebook %%% Distributed under the Thrift Software License -%%% +%%% %%% See accompanying file LICENSE or visit the Thrift site at: %%% http://developers.facebook.com/thrift/ diff --git a/lib/erl/src/transport/tErlAcceptor.erl b/lib/erl/src/transport/tErlAcceptor.erl index 83989240..bcfcb9a5 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 @@ accept(This, ListenSocket, GP, Handler) -> 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 @@ accept(This, ListenSocket, GP, Handler) -> 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 f002fd8e..5aead1e7 100644 --- a/lib/erl/src/transport/tSocket.erl +++ b/lib/erl/src/transport/tSocket.erl @@ -75,10 +75,8 @@ effectful_open(This) -> 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 @@ effectful_write(This, Str) -> 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 @@ read(This, Sz) -> {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 b7dc43d2..f81ae9b5 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 @@ super() -> %%% 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).