[thrift] retool oop.erl, fix tBufferedTransportFactory.erl
Summary: oop.erl used to assume that an undef or function_clause meant a method wasn't defined, but sometimes a method does exist and an undef happens while it's executing.
Case in point, getTransport in tBufferedTransportFactory totally didn't work and instead of exiting, oop.erl fell back silently to tTransportFactory, so everywhere I thought I was talking to tBufferedTransport, I was talking directly to the tSocket. borkborkbork.
Blame: all me baby
Reviewed By: eletuchy
Test Plan: channel server works
Revert Plan: ok
git-svn-id: https://svn.apache.org/repos/asf/incubator/thrift/trunk@665299 13f79535-47bb-0310-9956-ffa450edef68
diff --git a/lib/erl/src/oop.erl b/lib/erl/src/oop.erl
index 08c0e37..20edb86 100644
--- a/lib/erl/src/oop.erl
+++ b/lib/erl/src/oop.erl
@@ -4,143 +4,143 @@
%%% See accompanying file LICENSE or visit the Thrift site at:
%%% http://developers.facebook.com/thrift/
+%%%
+%%% dox:
+%%%
+%%% C++ <-> Erlang
+%%% classes modules
+%%% class b : public a a:super() -> b.
+%%%
+
-module(oop).
--export([get/2, set/3, call/2, call/3, inspect/1, start_new/2, is_object/1, class/1]).
+-export([start_new/2, get/2, set/3, call/2, call/3, inspect/1, class/1, is_object/1]).
+-export([call1/3]). %% only for thrift_oop_server ... don't use it
-export([behaviour_info/1]).
-include("thrift.hrl").
-include("oop.hrl").
+%% state for the call loop
+-record(cstate, {
+ obj, %% the current object (on which we want to invoke MFA)
+ module, %% the current module we're considering
+ func, %% the method name (i.e. the function we're trying to invoke in Module)
+ args, %% the arguments, the first of which is Obj
+ tried, %% a (backwards) list of modules we've tried
+ first_obj %% the original object
+ }).
+
%%%
%%% behavior definition
%%%
-behaviour_info(callbacks) ->
- [
- {attr, 4},
- {super, 0}
- ];
-behaviour_info(_) ->
+behaviour_info(callbacks) ->
+ [ {attr, 4},
+ {super, 0}
+ ];
+behaviour_info(_) ->
undefined.
-%%
+%%%
+%%% public interface
+%%%
--define(TRIED, lists:reverse([TryModule|TriedRev])).
+%% TODO: voids take only ok as return?
+start_new(none=Resv, _) ->
+ error_logger:format("can't instantiate ~p: class name is a reserved word", [Resv]),
+ error;
+start_new(Class, Args) ->
+ {ok, Pid} = gen_server:start_link(thrift_oop_server, {Class, Args}, []),
+ Pid.
-%% no super attr defined
--define(NOSUPEROBJ, exit({missing_attr_super, {inspect(Obj), ?TRIED}})).
-
--define(NOMETHOD, exit({missing_method, {Method, inspect(Obj), tl(Args), ?TRIED}})).
-
--define(NOATTR, exit({missing_attr, {hd(tl(Args)), inspect(FirstObj), ?TRIED}})).
-
--define(NOATTR_SET, exit({missing_attr, {Field, inspect(Obj), ".." %% TODO: give a backtrace
- }})).
-
-
-%%% get(Obj, Field) -> term()
-%%% looks up Field in Obj or its ancestor objects
-
+%% get(Obj, Field) -> term()
+%% looks up Field in Obj or its ancestor objects
get(Obj, Field) ->
call(Obj, attr, [get, Field, get]).
set(Obj, Field, Value) -> %% TODO: could be tail-recursive
Module = ?CLASS(Obj),
- try
- Module:attr(Obj, set, Field, Value)
- catch
- error:Kind when Kind == undef; Kind == function_clause ->
- case get_superobject(Obj) of
+ case apply_if_defined(Module, attr, [Obj, set, Field, Value]) of
+ {ok, V} -> V;
+ undef ->
+ case get_superobject(Obj) of
{ ok, Superobj } ->
- Super1 = set(Superobj, Field, Value),
- try
- Module:attr(Obj, set, super, Super1)
- catch %% TODO(cpiro): remove check
- X -> exit({burnsauce, X})
- end;
- none ->
- ?NOATTR_SET
+ Superobj1 = set(Superobj, Field, Value),
+ Module:attr(Obj, set, super, Superobj1);
+ undef ->
+ error(missing_attr_set, Field, Obj)
end
end.
-%%% C++ <-> Erlang
-%%% classes modules
-%%% class b : public a a:super() -> b.
-%%%
+%%
+%% ** dynamic method dispatch **
+%%
+%% calls Module:Func(*Args) if it exists
+%% if not, Module <- Module:super() and try again recursively
+%%
+%% Module:attr(*Args) is handled specially:
+%% Obj needs to be replaced with Obj's "superobject"
+%%
+call(Obj, Func) ->
+ call(Obj, Func, []).
-get_superobject(Obj) ->
- try
- {ok, (?CLASS(Obj)):attr(Obj, get, super, get)}
- catch
- error:Kind when Kind == undef; Kind == function_clause ->
- none
+call(Obj, Func, ArgsProper) ->
+ %% error_logger:info_msg("call called: Obj=~p Func=~p ArgsProper=~p", [inspect(Obj), Func, ArgsProper]),
+ case call1(Obj, Func, ArgsProper) of
+ {ok, Value} -> Value;
+ {error, Kind, S1} -> error(Kind, S1)
end.
-is_object(Obj) when is_tuple(Obj) ->
- try
- (?CLASS(Obj)):super(), %% if it's an object its first element will be a class name, and it'll have super/0
- true
- catch
- error:Kind when Kind == undef; Kind == function_clause ->
- false
- end;
-is_object(_) ->
- false.
+call1(Obj, Func, ArgsProper) ->
+ S = #cstate{
+ obj = Obj,
+ module = ?CLASS(Obj),
+ func = Func,
+ args = [Obj|ArgsProper], %% prepend This to args
+ tried = [],
+ first_obj = Obj
+ },
+ call1(S).
-call(Obj, Method, ArgsProper) ->
- %% error_logger:info_msg("call called: Obj=~p Method=~p ArgsProper=~p", [inspect(Obj), Method, ArgsProper]),
- Args = [Obj|ArgsProper], %% prepend This to args
- TryModule = ?CLASS(Obj),
- call_loop(Obj, Method, Args, TryModule, [], Obj).
-
-call(Obj, Method) ->
- call(Obj, Method, []).
-
-call_loop(Obj, Method, Args, TryModule, TriedRev, FirstObj) ->
- try
- %% error_logger:info_msg("call_loop~n ~p~n ~p~n ~p~n ~p", [inspect(Obj), Method, Args, TryModule]),
- apply(TryModule, Method, Args)
- catch
- error:Kind when Kind == undef; Kind == function_clause ->
- case { TryModule:super(), Method } of
- { none, attr } ->
- ?NOATTR;
-
- { none, _ } ->
- ?NOMETHOD;
-
- { Superclass, attr } ->
- %% look for attrs in the "super object"
-
- case get_superobject(Obj) of
- {ok, Superobj} when (TryModule == ?CLASS(Obj)) ->
- %% replace This with Superobj
- NewArgs = [Superobj|tl(Args)],
- call_loop(Superobj, Method, NewArgs,
- Superclass, [TryModule|TriedRev], FirstObj);
-
- {ok, _Superobj} -> % failed guard TODO(cpiro): removeme
- exit(oh_noes);
-
- none -> ?NOSUPEROBJ
- end;
-
- { SuperClass, _ } ->
- call_loop(Obj, Method, Args,
- SuperClass, [TryModule|TriedRev], FirstObj)
- end
+call1(S = #cstate{obj=Obj, module=Module, func=Func, args=Args}) ->
+ %% error_logger:info_msg("call1~n obj=~p~n MFA=~p, ~p, ~p", [inspect(Obj), Module, Func, Args]),
+ %% io:format("call ~p~n", [Module]),
+ case apply_if_defined(Module, Func, Args) of
+ {ok, Value} -> {ok, Value};
+ undef -> call1_try_super(S)
end.
-
-class(Obj) when is_tuple(Obj) ->
- case is_object(Obj) of
- true ->
- ?CLASS(Obj);
- false ->
- none
+
+call1_try_super(S = #cstate{func=attr, module=Module, tried=Tried}) ->
+ case Module:super() of
+ none -> {error, missing_attr, S};
+ Superclass -> call1_try_super_attr(Superclass, S)
end;
-class(_) ->
- none.
+call1_try_super(S = #cstate{func=Func, module=Module, tried=Tried}) ->
+ case Module:super() of
+ none -> {error, missing_method, S};
+ Superclass ->
+ S1 = S#cstate{
+ module = Superclass,
+ tried = [Module|Tried]
+ },
+ call1(S1)
+ end.
+
+call1_try_super_attr(Superclass, S = #cstate{obj=Obj, module=Module, args=Args, tried=Tried}) ->
+ %% look for attrs in the "super object"
+ case get_superobject(Obj) of
+ undef -> {error, missing_superobj, S};
+ {ok, Superobj} when Module == ?CLASS(Obj) ->
+ %% replace This with Superobj
+ S1 = S#cstate{
+ obj = Superobj,
+ args = [Superobj|tl(Args)],
+ module = Superclass,
+ tried = [Module|Tried]
+ },
+ call1(S1)
+ end.
%% careful: not robust against records beginning with a class name
%% (note: we can't just guard with is_record(?CLASS(Obj), Obj) since we
@@ -149,7 +149,7 @@
try
case is_object(Obj) of
true ->
- DeepList = inspect_loop(Obj, "#<"),
+ DeepList = inspect1(Obj, "#<"),
lists:flatten(DeepList);
false ->
thrift_utils:sformat("~p", [Obj])
@@ -162,22 +162,80 @@
%% _:E -> thrift_utils:sformat("~p", [Obj])
end.
-inspect_loop(Obj, Str) ->
+inspect1(Obj, Str) ->
Class = ?CLASS(Obj),
Inspect = Class:inspect(Obj),
Current = atom_to_list(Class) ++ ": " ++ Inspect,
case get_superobject(Obj) of
- { ok, Superobj } ->
- inspect_loop(Superobj, Str ++ Current ++ " | ");
- none ->
+ {ok, Superobj} ->
+ inspect1(Superobj, Str ++ Current ++ " | ");
+ undef ->
Str ++ Current ++ ">"
end.
-%% TODO: voids take only ok as return?
-start_new(none=Resv, _) ->
- error_logger:format("can't instantiate ~p: class name is a reserved word", [Resv]),
- error;
-start_new(Class, Args) ->
- {ok, Pid} = gen_server:start_link(thrift_oop_server, {Class, Args}, []),
- Pid.
+%% class(Obj) -> atom() = Class
+%% | none
+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;
+ undef -> none
+ end;
+class(_) -> none.
+
+%% is the tuple/record an object?
+%% is_object(Obj) = bool()
+is_object(Obj) when is_tuple(Obj) ->
+ case class(Obj) of
+ none -> false;
+ _ -> true
+ end;
+is_object(_) -> false.
+
+%%%
+%%% private helpers
+%%%
+
+%% apply_if_defined(MFA) -> {ok, apply(MFA)}
+%% | undef
+%% this could be worth some money
+apply_if_defined(M, F, A) ->
+ apply_if_defined({M,F,A}).
+
+apply_if_defined({M,F,A} = MFA) ->
+ try
+ %% io:format("apply ~p ~p ~p~n", [M,F,A]),
+ {ok, apply(M, F, A)}
+ catch
+ error: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
+ [MFA|_] -> undef; % trapped successfully
+ ST ->
+ io:format("DONIT THE EXIT THING ~p~n", [Kind]),
+ exit({Kind, ST}) % some unrelated error, re-exit
+ end
+ end.
+
+get_superobject(Obj) ->
+ apply_if_defined(?CLASS(Obj), attr, [Obj, get, super, get]).
+
+%%%
+%%% errors
+%%%
+
+tried(S = #cstate{module=Module, tried=Tried}) ->
+ lists:reverse([Module|Tried]).
+
+error(missing_superobj, S = #cstate{obj=Obj}) ->
+ exit({missing_superobj, {inspect(Obj), tried(S)}});
+error(missing_method, S = #cstate{obj=Obj, func=Func, args=Args}) ->
+ exit({missing_method, {Func, inspect(Obj), tl(Args), tried(S)}});
+error(missing_attr, S = #cstate{args=Args, first_obj=FirstObj}) ->
+ exit({missing_attr, {hd(tl(Args)), inspect(FirstObj), tried(S)}}).
+
+error(missing_attr_set, Field, Obj) ->
+ BT = "..", %% TODO: give a backtrace
+ exit({missing_attr, {Field, inspect(Obj), BT}}).
diff --git a/lib/erl/src/protocol/tBinaryProtocol.erl b/lib/erl/src/protocol/tBinaryProtocol.erl
index b745298..9430e39 100644
--- a/lib/erl/src/protocol/tBinaryProtocol.erl
+++ b/lib/erl/src/protocol/tBinaryProtocol.erl
@@ -113,28 +113,28 @@
writeByte(This, Byte) ->
Trans = oop:get(This, trans),
- ?R1(Trans, write, binary_to_list(<<Byte:8/big>>)).
+ ?R1(Trans, effectful_write, binary_to_list(<<Byte:8/big>>)).
writeI16(This, I16) ->
Trans = oop:get(This, trans),
- ?R1(Trans, write, binary_to_list(<<I16:16/big>>)).
+ ?R1(Trans, effectful_write, binary_to_list(<<I16:16/big>>)).
writeI32(This, I32) ->
Trans = oop:get(This, trans),
- ?R1(Trans, write, binary_to_list(<<I32:32/big>>)).
+ ?R1(Trans, effectful_write, binary_to_list(<<I32:32/big>>)).
writeI64(This, I64) ->
Trans = oop:get(This, trans),
- ?R1(Trans, write, binary_to_list(<<I64:64/big>>)).
+ ?R1(Trans, effectful_write, binary_to_list(<<I64:64/big>>)).
writeDouble(This, Double) ->
Trans = oop:get(This, trans),
- ?R1(Trans, write, binary_to_list(<<Double:64/big>>)).
+ ?R1(Trans, effectful_write, binary_to_list(<<Double:64/big>>)).
writeString(This, Str) ->
Trans = oop:get(This, trans),
?L1(writeI32, length(Str)),
- ?R1(Trans, write, Str).
+ ?R1(Trans, effectful_write, Str).
%
diff --git a/lib/erl/src/transport/tBufferedTransport.erl b/lib/erl/src/transport/tBufferedTransport.erl
index 48d9fba..3c0a046 100644
--- a/lib/erl/src/transport/tBufferedTransport.erl
+++ b/lib/erl/src/transport/tBufferedTransport.erl
@@ -39,7 +39,7 @@
%%% inspect(This) -> string()
inspect(This) ->
- ?FORMAT_ATTR(transport) ++
+ ?FORMAT_ATTR(transport) ++ ", " ++
?FORMAT_ATTR(wbuf).
%%%
diff --git a/lib/erl/src/transport/tBufferedTransportFactory.erl b/lib/erl/src/transport/tBufferedTransportFactory.erl
index 9746aba..5bc5012 100644
--- a/lib/erl/src/transport/tBufferedTransportFactory.erl
+++ b/lib/erl/src/transport/tBufferedTransportFactory.erl
@@ -51,4 +51,4 @@
%%%
getTransport(_This, Trans) ->
- gen_server:start_link(tBufferedTransport, {new, [Trans]}).
+ oop:start_new(tBufferedTransport, [Trans]).
diff --git a/lib/erl/src/transport/tSocket.erl b/lib/erl/src/transport/tSocket.erl
index c2bf920..c438203 100644
--- a/lib/erl/src/transport/tSocket.erl
+++ b/lib/erl/src/transport/tSocket.erl
@@ -19,7 +19,7 @@
-export([new/0, new/1, new/2,
effectful_setHandle/2, effectful_open/1,
- isOpen/1, write/2, read/2, effectful_close/1]).
+ isOpen/1, effectful_write/2, read/2, effectful_close/1]).
%%%
%%% define attributes
@@ -87,7 +87,7 @@
isOpen(This) ->
oop:get(This, handle) /= nil.
-write(This, Str) ->
+effectful_write(This, Str) ->
Handle = oop:get(This, handle),
Val = gen_tcp:send(Handle, Str),
@@ -100,7 +100,7 @@
{error, _} ->
throw(tTransportException:new(?tTransportException_NOT_OPEN, "in write"));
ok ->
- ok
+ {ok, This}
end.
read(This, Sz) ->
diff --git a/lib/erl/src/transport/tTransport.erl b/lib/erl/src/transport/tTransport.erl
index 6ec115a..a7293be 100644
--- a/lib/erl/src/transport/tTransport.erl
+++ b/lib/erl/src/transport/tTransport.erl
@@ -50,12 +50,13 @@
%%% instance methods
%%%
+e() ->
+ exit('tTransport is abstract').
-
-isOpen(_This) -> nil.
-open(_This) -> nil.
-close(_This) -> nil.
-read(_This, _Sz) -> nil.
+isOpen(_This) -> e(), nil.
+open(_This) -> e(), nil.
+close(_This) -> e(), nil.
+read(_This, _Sz) -> e(), nil.
readAll(This, Sz) ->
readAll_loop(This, Sz, "", 0).
@@ -80,6 +81,7 @@
end.
effectful_write(This, _Buf) ->
+ e(),
{nil, This}.
effectful_flush(This) ->