Skip to content

Commit

Permalink
Removing error fun grom gcm.erl and therefore from gcm_sup.erl
Browse files Browse the repository at this point in the history
  • Loading branch information
Paolo D'Incau committed Dec 23, 2014
1 parent f767c3f commit f7f3587
Show file tree
Hide file tree
Showing 3 changed files with 29 additions and 83 deletions.
100 changes: 23 additions & 77 deletions src/gcm.erl
View file
Original file line number Diff line number Diff line change
@@ -1,22 +1,19 @@
-module(gcm).
-behaviour(gen_server).

-export([start/2, start/3, stop/1, start_link/2, start_link/3]).
-export([start/2, stop/1, start_link/2]).

-export([init/1, handle_call/3, handle_cast/2, handle_info/2,
terminate/2, code_change/3]).

-export([push/3, sync_push/3, update_error_fun/2]).
-export([push/3, sync_push/3]).

-define(SERVER, ?MODULE).

-record(state, {key, error_fun}).
-record(state, {key}).

start(Name, Key) ->
start(Name, Key, fun log_error/2).

start(Name, Key, ErrorFun) ->

This comment has been minimized.

Sign in to view

Copy link
@cclam0827

cclam0827 Jan 15, 2015

may I know why remove error_fun API?

This comment has been minimized.

Sign in to view

Copy link
@pdincau

pdincau Jan 15, 2015

Owner

error_fun was used to deal with error messages in an ordinate way in order to log or retry with a custom function. In master now errors are logged by default and in case of 50X status the message is automatically sent again. For all other errors I plan to store them in a ets table that the user will be able to poll.
If this is not ok, I can consider reintroducing error_fun.

Why do you think error_fun should be kept? What is the use case you want to solve? Feedback from people is more that welcome.

BTW if you refer to tag 1.0.0 error_fun is still available.

This comment has been minimized.

Sign in to view

Copy link
@cclam0827

cclam0827 Jan 15, 2015

Thanks for your consideration. For now we changed to use 1.0.0, we used err_fun to handle some invalid / Updated Token to update our user database record.

This comment has been minimized.

Sign in to view

Copy link
@pdincau

pdincau Jan 15, 2015

Owner

What about storing the registered ids to update in an ets table and poll it? This could be a feature on next milestone. Let me know

This comment has been minimized.

Sign in to view

Copy link
@cclam0827

cclam0827 Jan 15, 2015

let me clarify what you mean, is that you want to store the registered ids to ets table then user can poll it from the "local_content" ets table right? I guess it depends, for me I prefer callback fun is better because I can handle it in other nodes. Or why not keep both method? What do you think?

This comment has been minimized.

Sign in to view

Copy link
@pdincau

pdincau Jan 15, 2015

Owner

Exactly.
Reintroducing error_fun is not a problem for me. There are different approach though: for example one can pass a pid to each gcm_erlang instance and gcm_can notify back when a new registration id is available for that device. In this way gcm_erlang will handle the push logic but won't deal with users data. How does it sound?

This comment has been minimized.

Sign in to view

Copy link
@pdincau

pdincau Jan 15, 2015

Owner

But probably you are already passing a function that does something like that. Let me see how error_fun can be reintroduced

This comment has been minimized.

Sign in to view

Copy link
@cclam0827

cclam0827 Jan 15, 2015

I have a thought, is it possible to pass Ref_ID or some Ref Key with the Token? Such as I pass the Token to gcm with user ID, and the call back will include the user ID or Ref_ID. Then I can handle it using user ID / Ref ID to process something...the ID is used for recognise the Token belong to somebody..

This comment has been minimized.

Sign in to view

Copy link
@cclam0827

cclam0827 Jan 15, 2015

Sample,

gcm:push(RegisteredName, Reference, RegIds, Payload)

And the callback (err fun) include the Reference

This comment has been minimized.

Sign in to view

Copy link
@pdincau

pdincau Jan 15, 2015

Owner

Not very clear what you want to achieve. I opened an issue #14 so that we can talk there in a better way.
gcm_sup:start_child(Name, Key, ErrorFun).
gcm_sup:start_child(Name, Key).

stop(Name) ->
gen_server:call(Name, stop).
Expand All @@ -27,38 +24,28 @@ push(Name, RegIds, Message) ->
sync_push(Name, RegIds, Message) ->
gen_server:call(Name, {send, RegIds, Message}).

update_error_fun(Name, Fun) ->
gen_server:cast(Name, {error_fun, Fun}).

%% OTP
start_link(Name, Key) ->
start_link(Name, Key, fun log_error/2).

start_link(Name, Key, ErrorFun) ->
gen_server:start_link({local, Name}, ?MODULE, [Key, ErrorFun], []).
gen_server:start_link({local, Name}, ?MODULE, [Key], []).

init([Key, ErrorFun]) ->
{ok, #state{key=Key, error_fun=ErrorFun}}.
init([Key]) ->
{ok, #state{key=Key}}.

handle_call(stop, _From, State) ->
{stop, normal, stopped, State};

handle_call({send, RegIds, Message}, _From, #state{key=Key} = State) ->
Reply = do_push(RegIds, Message, Key, undefined),
Reply = do_push(RegIds, Message, Key),
{reply, Reply, State};

handle_call(_Request, _From, State) ->
Reply = ok,
{reply, Reply, State}.

handle_cast({send, RegIds, Message}, #state{key=Key, error_fun=ErrorFun} = State) ->
do_push(RegIds, Message, Key, ErrorFun),
handle_cast({send, RegIds, Message}, #state{key=Key} = State) ->
do_push(RegIds, Message, Key),
{noreply, State};

handle_cast({error_fun, Fun}, State) ->
NewState = State#state{error_fun=Fun},
{noreply, NewState};

handle_cast(_Msg, State) ->
{noreply, State}.

Expand All @@ -72,85 +59,44 @@ code_change(_OldVsn, State, _Extra) ->
{ok, State}.

%% Internal
do_push(RegIds, Message, Key, ErrorFun) ->
do_push(RegIds, Message, Key) ->
error_logger:info_msg("Sending message: ~p to reg ids: ~p~n", [Message, RegIds]),
case gcm_api:push(RegIds, Message, Key) of
{ok, GCMResult} ->
handle_result(GCMResult, RegIds, ErrorFun);
handle_result(GCMResult, RegIds);
{error, {retry, RetryTime}} ->
do_backoff(RetryTime, RegIds, Message, Key, ErrorFun),
do_backoff(RetryTime, RegIds, Message, Key),
{error, retry};
{error, Reason} ->
{error, Reason}
end.

handle_result(GCMResult, RegIds, undefined) ->
handle_result(GCMResult, RegIds) ->
{_MulticastId, _SuccessesNumber, _FailuresNumber, _CanonicalIdsNumber, Results} = GCMResult,
lists:map(fun({Result, RegId}) ->
parse_result(Result, RegId, fun(E, I) -> {E, I} end)
end, lists:zip(Results, RegIds));

handle_result(GCMResult, RegIds, ErrorFun) ->
{_MulticastId, _SuccessesNumber, FailuresNumber, CanonicalIdsNumber, Results} = GCMResult,
case to_be_parsed(FailuresNumber, CanonicalIdsNumber) of
true ->
lists:foreach(fun({Result, RegId}) -> parse_result(Result, RegId, ErrorFun) end,
lists:zip(Results, RegIds));
false ->
ok
end.
parse_result(Result, RegId)
end, lists:zip(Results, RegIds)).

do_backoff(RetryTime, RegIds, Message, Key, ErrorFun) ->
do_backoff(RetryTime, RegIds, Message, Key) ->
case RetryTime of
no_retry ->
ok;
Time ->
timer:apply_after(Time * 1000, ?MODULE, do_push, [RegIds, Message, Key, ErrorFun])
timer:apply_after(Time * 1000, ?MODULE, do_push, [RegIds, Message, Key])
end.

to_be_parsed(0, 0) -> false;

to_be_parsed(_FailureNumber, _CanonicalNumber) -> true.

parse_result(Result, RegId, ErrorFun) ->
parse_result(Result, RegId) ->
case {
proplists:get_value(<<"error">>, Result),
proplists:get_value(<<"message_id">>, Result),
proplists:get_value(<<"registration_id">>, Result)
} of
{Error, undefined, undefined} when Error =/= undefined ->
ErrorFun(Error, RegId);
error_logger:info_msg("Error: ~p for registered id: ~p~n", [Error, RegId]),
{RegId, Error};
{undefined, MessageId, undefined} when MessageId =/= undefined ->
ok;
{undefined, MessageId, NewRegId} when MessageId =/= undefined andalso NewRegId =/= undefined ->
ErrorFun(<<"NewRegistrationId">>, {RegId, NewRegId})
error_logger:info_msg("Message sent. Update id ~p with new id ~p.~n", [RegId, NewRegId]),
{RegId, {<<"NewRegistrationId">>, NewRegId}}
end.

log_error(<<"NewRegistrationId">>, {RegId, NewRegId}) ->
error_logger:info_msg("Message sent. Update id ~p with new id ~p.~n", [RegId, NewRegId]),
ok;

log_error(<<"Unavailable">>, RegId) ->
%% The server couldn't process the request in time. Retry later with exponential backoff.
error_logger:error_msg("unavailable ~p~n", [RegId]),
ok;

log_error(<<"InternalServerError">>, RegId) ->
% GCM had an internal server error. Retry later with exponential backoff.
error_logger:error_msg("internal server error ~p~n", [RegId]),
ok;

log_error(<<"InvalidRegistration">>, RegId) ->
%% Invalid registration id in database.
error_logger:error_msg("invalid registration ~p~n", [RegId]),
ok;

log_error(<<"NotRegistered">>, RegId) ->
%% Application removed. Delete device from database.
error_logger:error_msg("not registered ~p~n", [RegId]),
ok;

log_error(UnexpectedError, RegId) ->
%% There was an unexpected error that couldn't be identified.
error_logger:error_msg("unexpected error ~p in ~p~n", [UnexpectedError, RegId]),
ok.
8 changes: 4 additions & 4 deletions src/gcm_sup.erl
View file
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
-module(gcm_sup).
-behaviour(supervisor).

-export([start_link/0, start_child/3]).
-export([start_link/0, start_child/2]).

-export([init/1]).

Expand All @@ -11,10 +11,10 @@
start_link() ->
supervisor:start_link({local, ?MODULE}, ?MODULE, []).

-spec start_child(atom(),string(),fun()) ->
-spec start_child(atom(),string()) ->
{'error',_} | {'ok','undefined' | pid()} | {'ok','undefined' | pid(),_}.
start_child(Name, ApiKey, ErrorFun) ->
supervisor:start_child(?MODULE, [Name, ApiKey, ErrorFun]).
start_child(Name, ApiKey) ->
supervisor:start_child(?MODULE, [Name, ApiKey]).

-spec init([]) -> {ok, {{supervisor:strategy(), 5, 10}, [supervisor:child_spec()]}}.
init([]) ->
Expand Down
4 changes: 2 additions & 2 deletions test/gcm_test.erl
View file
Original file line number Diff line number Diff line change
Expand Up @@ -88,8 +88,8 @@ receive_results_from_sync_push(_) ->
end),
Result = gcm:sync_push(test, [<<"Token0">>, <<"Token1">>, <<"Token2">>],
[{<<"data">>, [{<<"type">>, <<"wakeUp">>}]}]),
ExpectedResult = [ok, {<<"InvalidRegistration">>, <<"Token1">>},
{<<"NewRegistrationId">>, {<<"Token2">>, <<"NewToken">>}}],
ExpectedResult = [ok, {<<"Token1">>, <<"InvalidRegistration">>},
{<<"Token2">>, {<<"NewRegistrationId">>, <<"NewToken">>}}],
[
{"Results are passed to the caller", ?_assertMatch(ExpectedResult, Result)},
{"Validate httpc", ?_assert(meck:validate(httpc))}
Expand Down

0 comments on commit f7f3587

Please sign in to comment.