Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add exceptions #20

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
56 changes: 46 additions & 10 deletions lib/riffed/client.ex
Original file line number Diff line number Diff line change
Expand Up @@ -59,32 +59,55 @@ defmodule Riffed.Client do
end
end

defp build_exception_handlers(exception_type, struct_module) do
{_seq, struct_def={:struct, {thrift_struct_module, exception_name}}} = exception_type
{:struct, detailed_meta} = :erlang.apply(thrift_struct_module, :struct_info_ext, [exception_name])
params = build_arg_list(Enum.count(detailed_meta), "_")
|> List.insert_at(0, exception_name)

args = {:{}, [], params}

quote do
defp raise_exception(ex=unquote(args)) do
ex = unquote(struct_module).to_elixir(ex, unquote(struct_def))
raise ex
end
end
end

defp build_client_function(thrift_metadata, struct_module, function_name, overrides) do
function_meta = Meta.metadata_for_function(thrift_metadata, function_name)
param_meta = function_meta[:params]
exception_meta = function_meta[:exceptions]
reply_meta = function_meta[:reply] |> Riffed.Struct.to_riffed_type_spec

reply_meta = Riffed.Enumeration.get_overridden_type(function_name, :return_type, overrides, reply_meta)

arg_list = build_arg_list(length(param_meta))
{:{}, _, list_args} = build_handler_tuple_args(param_meta)
casts = build_casts(function_name, struct_module, param_meta, overrides, :to_erlang)
exception_handlers = exception_meta
|> Enum.map(&build_exception_handlers(&1, struct_module))

quote do
def unquote(function_name)(unquote_splicing(arg_list)) do
unquote_splicing(casts)

rv = GenServer.call(__MODULE__, {unquote(function_name), unquote(list_args)})
unquote(struct_module).to_elixir(rv, unquote(reply_meta))
end
unquote_splicing(exception_handlers)

def unquote(function_name)(client_pid, unquote_splicing(arg_list))
when is_pid(client_pid) do

unquote_splicing(casts)

rv = GenServer.call(client_pid, {unquote(function_name), unquote(list_args)})
unquote(struct_module).to_elixir(rv, unquote(reply_meta))
case rv do
{:exception, exception_record} ->
raise_exception(exception_record)
success ->
unquote(struct_module).to_elixir(success, unquote(reply_meta))
end
end

def unquote(function_name)(unquote_splicing(arg_list)) do
__MODULE__
|> Process.whereis
|> unquote(function_name)(unquote_splicing(arg_list))
end
end
end
Expand Down Expand Up @@ -185,6 +208,11 @@ defmodule Riffed.Client do

unquote_splicing(client_functions)

# the default no-op for functions that don't have exceptions.
defp raise_exception(e) do
e
end

def handle_call({call_name, args}, _parent, client) do
{new_client, response} = call_thrift(client, call_name, args)
{:reply, response, new_client}
Expand All @@ -200,15 +228,22 @@ defmodule Riffed.Client do

defp call_thrift(client, call_name, args, retry_count)
when retry_count < unquote(num_retries) do
{thrift_client, response} =
try do
:thrift_client.call(client.client, call_name, args)
catch {new_client, exception} ->
{new_client, exception}
end

{thrift_client, response} = :thrift_client.call(client.client, call_name, args)
new_client = %Client{client | client: thrift_client}
case response do
{:error, :closed} ->
new_client = Client.reconnect(client)
call_thrift(new_client, call_name, args, retry_count + 1)
err = {:error, _} ->
{new_client, err}
exception = {:exception, exception_record} ->
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've found that this isn't enough to catch the exception.. The actual erlang thrift client will throw the exception, not just return it. So when working with services that go through the thrift client (maybe the tests here don't?), the GenServer will EXIT before we reach this case statement

https://github.com/apache/thrift/blob/master/lib/erl/src/thrift_client.erl#L150

        [Exception] ->
            throw({NewClient, {exception, Exception}})

I had to modify the :thrift_client.call to look something like this:

        {thrift_client, response} = try do
          :thrift_client.call(client.client, call_name, args)
        catch
          exception_response = {thrift_client, {:exception, exception}} -> exception_response
        end

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@binaryseed is this the behavior in 0.9.3 or in head? They made a bunch of changes in head (and in the forever-upcoming 0.10 release)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe so, the code I reference is in master and I'm running based of a fork of elixir-thrift that points at master, not 0.9.3

{:new_client, exception}
{:ok, rsp} ->
{new_client, rsp}
other = {other, rsp} ->
Expand Down Expand Up @@ -252,4 +287,5 @@ defmodule Riffed.Client do
end
end
end

end
12 changes: 7 additions & 5 deletions lib/riffed/macro_helpers.ex
Original file line number Diff line number Diff line change
@@ -1,14 +1,16 @@
defmodule Riffed.MacroHelpers do
@moduledoc false
def build_arg_list(size) when is_integer(size) do

def build_arg_list(size, prefix \\ nil) when is_integer(size) do
case size do
0 -> []
size ->
Enum.map(1..size, fn(param_idx) ->
:"arg_#{param_idx}"
|> Macro.var(nil)
end)
Enum.map(1..size, fn(param_idx) ->
:"#{prefix}arg_#{param_idx}"
|> Macro.var(nil)
end)
end

end

def build_handler_tuple_args(param_meta) do
Expand Down
40 changes: 36 additions & 4 deletions lib/riffed/server.ex
Original file line number Diff line number Diff line change
Expand Up @@ -110,25 +110,57 @@ defmodule Riffed.Server do
[delegate_info[:module]]}, delegate_info[:name]]}, [], arg_list}
end

defp wrap_with_exception_handlers([], _struct_module, handler_call) do
handler_call
end

defp wrap_with_exception_handlers(exceptions, struct_module, handler_call) do
elixir_exceptions = exceptions
|> Enum.map(fn({_seq,{:struct, {_thrift_module, ex_name}}}) ->
Module.concat(struct_module, ex_name)
end)

quote do
try do
unquote(handler_call)

rescue e in unquote(elixir_exceptions) -> e
throw unquote(struct_module).to_erlang(e, nil)
end
end
end

defp build_handler(meta=%Meta{}, struct_module, thrift_fn_name, delegate_fn, fn_overrides) do

function_meta = Meta.metadata_for_function(meta, thrift_fn_name)
params_meta = function_meta[:params]
reply_meta = function_meta[:reply] |> Riffed.Struct.to_riffed_type_spec
exception_meta = function_meta[:exceptions]
tuple_args = build_handler_tuple_args(params_meta)
delegate_call = build_delegate_call(delegate_fn)
casts = build_casts(thrift_fn_name, struct_module, params_meta, fn_overrides, :to_elixir)
overridden_type = Riffed.Enumeration.get_overridden_type(thrift_fn_name, :return_type, fn_overrides, reply_meta)

exception_handling = wrap_with_exception_handlers(
exception_meta,
struct_module,
quote do
unquote(delegate_call)
|> unquote(struct_module).to_erlang(unquote(overridden_type))
end)

quote do
def handle_function(unquote(thrift_fn_name), unquote(tuple_args)) do
unquote_splicing(casts)
rsp = unquote(delegate_call)
|> unquote(struct_module).to_erlang(unquote(overridden_type))
rsp = unquote(exception_handling)

case rsp do
:ok -> :ok
_ -> {:reply, rsp}
:ok ->
:ok
exc = {:exception, ex} ->
exc
_ ->
{:reply, rsp}
end
end
end
Expand Down
55 changes: 51 additions & 4 deletions lib/riffed/struct.ex
Original file line number Diff line number Diff line change
Expand Up @@ -69,7 +69,7 @@ defmodule Riffed.Struct do

defmodule StructData do
@moduledoc false
defstruct struct_modules: [], tuple_converters: [], struct_converters: []
defstruct struct_modules: [], tuple_converters: [], struct_converters: [], exception_modules: []

def append(data=%StructData{}, struct_module, tuple_stanza, struct_function) do
%StructData{struct_modules: [struct_module | data.struct_modules],
Expand Down Expand Up @@ -172,11 +172,44 @@ defmodule Riffed.Struct do
defstruct unquote(struct_args)

def new(opts \\ unquote(struct_args)) do
Enum.reduce(opts, %unquote(fq_module_name){}, fn({k, v}, s) -> Map.put(s, k, v) end)
opts
|> Enum.reduce(%unquote(fq_module_name){}, fn({k, v}, s) -> Map.put(s, k, v) end)
end
end
end
StructData.append(struct_data, struct_module, tuple_to_elixir, struct_to_erlang)

struct_data
|> StructData.append(struct_module, tuple_to_elixir, struct_to_erlang)
end

def build_exception_and_conversion_function(struct_data=%StructData{}, root_module, container_module, exception_module_name, thrift_module) do

{:struct, meta} = :erlang.apply(thrift_module, :struct_info_ext, [exception_module_name])
exception_args = build_struct_args(meta)
fq_module_name = Module.concat([container_module, exception_module_name])
record_name = downcase_first(exception_module_name)
record_file = "src/#{thrift_module}.hrl"
tuple_to_elixir = build_tuple_to_elixir(thrift_module, root_module, fq_module_name, meta, exception_module_name)
struct_to_erlang = build_struct_to_erlang(root_module, fq_module_name, meta, exception_module_name, record_name)

exception_module = quote do
defmodule unquote(fq_module_name) do
require Record

Record.defrecord(unquote(record_name),
Record.extract(unquote(exception_module_name), from: unquote(record_file)))

defexception unquote(exception_args)

def new(opts \\ unquote(exception_args)) do
opts
|> Enum.reduce(%unquote(fq_module_name){}, fn({k, v}, s) -> Map.put(s, k, v) end)
end
end
end

struct_data
|> StructData.append(exception_module, tuple_to_elixir, struct_to_erlang)
end

@doc false
Expand Down Expand Up @@ -279,6 +312,14 @@ defmodule Riffed.Struct do
end
end

def partition_structs_and_exceptions(thrift_module, module_names) do
struct_names = :erlang.apply(thrift_module, :struct_names, [])
|> Enum.into(MapSet.new)

module_names
|> Enum.partition(&MapSet.member?(struct_names, &1))
end

defmacro __before_compile__(env) do
options = Module.get_attribute(env.module, :thrift_options)
build_cast_to_erlang = Module.get_attribute(env.module, :build_cast_to_erlang)
Expand All @@ -288,17 +329,23 @@ defmodule Riffed.Struct do
|> Enum.reduce(
%StructData{},
fn({thrift_module, struct_names}, data) ->
{structs, exceptions} = partition_structs_and_exceptions(thrift_module,
struct_names)
curr_module = env.module
dest_module = case Keyword.get(module_mapping, thrift_module, env.module) do
^curr_module ->
curr_module
override_module ->
Module.concat([env.module, override_module])
end
Enum.reduce(struct_names, data,
data = structs
|> Enum.reduce(data,
fn(struct_name, data) ->
build_struct_and_conversion_function(data, env.module, dest_module, struct_name, thrift_module)
end)
exceptions
|> Enum.reduce(data, &(build_exception_and_conversion_function(&2, env.module, dest_module, &1, thrift_module)))

end)

callbacks = Riffed.Callbacks.build(env.module)
Expand Down
20 changes: 10 additions & 10 deletions mix.lock
Original file line number Diff line number Diff line change
@@ -1,16 +1,16 @@
%{"certifi": {:hex, :certifi, "0.3.0"},
"earmark": {:hex, :earmark, "0.1.19"},
"ex_doc": {:hex, :ex_doc, "0.11.2"},
%{"certifi": {:hex, :certifi, "0.3.0", "389d4b126a47895fe96d65fcf8681f4d09eca1153dc2243ed6babad0aac1e763", [:rebar3], []},
"earmark": {:hex, :earmark, "0.1.19", "ffec54f520a11b711532c23d8a52b75a74c09697062d10613fa2dbdf8a9db36e", [:mix], []},
"ex_doc": {:hex, :ex_doc, "0.11.2", "8ac82c6144a27faca6a623eeebfbf5a791bc20a54ce29a9c02e499536d253d9b", [:mix], [{:earmark, "~> 0.1.17 or ~> 0.2", [hex: :earmark, optional: true]}]},
"excoveralls": {:git, "https://github.com/parroty/excoveralls.git", "a0d3d57b7137f3204690e4126a82d48d985a09fb", [tag: "v0.4.5"]},
"exjsx": {:hex, :exjsx, "3.2.0"},
"exjsx": {:hex, :exjsx, "3.2.0", "7136cc739ace295fc74c378f33699e5145bead4fdc1b4799822d0287489136fb", [:mix], [{:jsx, "~> 2.6.2", [hex: :jsx, optional: false]}]},
"exlager": {:git, "https://github.com/khia/exlager.git", "abb13435a540731b8b9c5f649f4d94f11a27c1f7", []},
"goldrush": {:git, "git://github.com/DeadZen/goldrush.git", "64864ba7fcf40988361340e48680b49a2c2938cf", [tag: "0.1.7"]},
"hackney": {:hex, :hackney, "1.4.8"},
"idna": {:hex, :idna, "1.0.3"},
"jsx": {:hex, :jsx, "2.6.2"},
"hackney": {:hex, :hackney, "1.4.8", "c8c6977ed55cc5095e3929f6d94a6f732dd2e31ae42a7b9236d5574ec3f5be13", [:rebar3], [{:certifi, "0.3.0", [hex: :certifi, optional: false]}, {:idna, "1.0.3", [hex: :idna, optional: false]}, {:mimerl, "1.0.2", [hex: :mimerl, optional: false]}, {:ssl_verify_hostname, "1.0.5", [hex: :ssl_verify_hostname, optional: false]}]},
"idna": {:hex, :idna, "1.0.3", "d456a8761cad91c97e9788c27002eb3b773adaf5c893275fc35ba4e3434bbd9b", [:rebar3], []},
"jsx": {:hex, :jsx, "2.6.2", "213721e058da0587a4bce3cc8a00ff6684ced229c8f9223245c6ff2c88fbaa5a", [:mix, :rebar], []},
"lager": {:git, "https://github.com/basho/lager.git", "310ed140ab86b95dd9f1f1ed3bf2ca93f841349d", []},
"meck": {:hex, :meck, "0.8.3"},
"mimerl": {:hex, :mimerl, "1.0.2"},
"meck": {:hex, :meck, "0.8.3", "4628a1334c69610c5bd558b04dc78d723d8ec5445c123856de34c77f462b5ee5", [:rebar], []},
"mimerl": {:hex, :mimerl, "1.0.2", "993f9b0e084083405ed8252b99460c4f0563e41729ab42d9074fd5e52439be88", [:rebar3], []},
"mock": {:git, "https://github.com/jjh42/mock.git", "c2b7dc75bc219124f8b458b62cf9a65616907e04", []},
"ssl_verify_hostname": {:hex, :ssl_verify_hostname, "1.0.5"},
"ssl_verify_hostname": {:hex, :ssl_verify_hostname, "1.0.5", "2e73e068cd6393526f9fa6d399353d7c9477d6886ba005f323b592d389fb47be", [:make], []},
"thrift": {:git, "https://github.com/pinterest/elixir-thrift.git", "9c3871cc9568eaf23c701ae11dece7bb5790b8ab", [tag: "1.0.0", submodules: true]}}
18 changes: 17 additions & 1 deletion test/riffed/client_test.exs
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,8 @@ defmodule ClientTest do
:getUserStates,
:getAllStates,
:getUsers,
:functionWithoutNumberedArgs
:functionWithoutNumberedArgs,
:callAndBlowUp
]

defenum ActivityState do
Expand Down Expand Up @@ -92,6 +93,13 @@ defmodule ClientTest do
end
end

def error_with(error) do
fn(client, fn_name, args) ->
EchoServer.set_args({fn_name, args})
{client, {:exception, error}}
end
end

test "it should convert nested structs into erlang" do
converted = Models.to_erlang(config_request_struct, {:struct, {:models, :ConfigRequest}})
assert {:ConfigRequest, "foo/bar", 32, {:User, "Foobie", "Barson", 1}} == converted
Expand Down Expand Up @@ -216,4 +224,12 @@ defmodule ClientTest do
assert response == 1234
assert {:functionWithoutNumberedArgs, [user_tuple, 23]} == last_call
end

test_with_mock "it should throw exceptions from the thrift server", :thrift_client, [call: error_with({:ServerException, "Server died", 500})] do

assert_raise Models.ServerException, fn ->
Client.callAndBlowUp("foo", "bar")
end
end

end
17 changes: 15 additions & 2 deletions test/riffed/integration_test.exs
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,10 @@ defmodule IntegrationTest do
use Riffed.Server, service: :server_thrift,
structs: IntegServer.Models,
functions: [getUserStates: &IntegServer.Handlers.get_user_states/1,
echoString: &IntegServer.Handlers.echo_string/1],
echoString: &IntegServer.Handlers.echo_string/1,
callAndBlowUp: &IntegServer.Handlers.call_and_blow_up/2
],

server: {
:thrift_socket_server,
port: 22831,
Expand Down Expand Up @@ -33,6 +36,10 @@ defmodule IntegrationTest do
def echo_string(input) do
input
end

def call_and_blow_up(message, _) do
raise IntegServer.Models.UsageException.new(message: message, code: 999)
end
end
end

Expand Down Expand Up @@ -61,7 +68,7 @@ defmodule IntegrationTest do
framed: true,
retries: 3],
service: :server_thrift,
import: [:getUserStates, :echoString]
import: [:getUserStates, :echoString, :callAndBlowUp]
end

defmodule EnumerizedClient do
Expand Down Expand Up @@ -179,4 +186,10 @@ defmodule IntegrationTest do
assert nil == HostAndPortClient.echoString(client, nil)
end

test "can handle exceptions" do
EasyClient.start_link
assert_raise EasyClient.Models.UsageException, fn ->
EasyClient.callAndBlowUp("foo", "bar")
end
end
end
Loading