From 65153795869eda29ccd1bb0d3b5c14e390681f4f Mon Sep 17 00:00:00 2001 From: Anders Svensson Date: Mon, 19 Jan 2015 11:45:40 +0100 Subject: Monitor more efficiently at shutdown There's no need for building a pid list only to map it to a list of monitor references. Also, monitoring before banging the shutdown message makes for better trace, avoiding unnecessary noproc reasons when the process dies before the monitor is created. --- lib/diameter/src/base/diameter_lib.erl | 18 ++++++++++-------- lib/diameter/src/base/diameter_service.erl | 5 +++-- 2 files changed, 13 insertions(+), 10 deletions(-) diff --git a/lib/diameter/src/base/diameter_lib.erl b/lib/diameter/src/base/diameter_lib.erl index 5b3a2063f8..b5f1ae3937 100644 --- a/lib/diameter/src/base/diameter_lib.erl +++ b/lib/diameter/src/base/diameter_lib.erl @@ -1,7 +1,7 @@ %% %% %CopyrightBegin% %% -%% Copyright Ericsson AB 2010-2014. All Rights Reserved. +%% Copyright Ericsson AB 2010-2015. All Rights Reserved. %% %% The contents of this file are subject to the Erlang Public License, %% Version 1.1, (the "License"); you may not use this file except in @@ -247,17 +247,19 @@ opts(HeapSize, Opts) -> %% # wait/1 %% --------------------------------------------------------------------------- --spec wait([pid()]) +-spec wait([pid() | reference()]) -> ok. wait(L) -> - down([erlang:monitor(process, P) || P <- L]). + lists:foreach(fun down/1, L). -down([]) -> - ok; -down([MRef|T]) -> - receive {'DOWN', MRef, process, _, _} -> ok end, - down(T). +down(Pid) + when is_pid(Pid) -> + down(monitor(process, Pid)); + +down(MRef) + when is_reference(MRef) -> + receive {'DOWN', MRef, process, _, _} = T -> T end. %% --------------------------------------------------------------------------- %% # fold_tuple/3 diff --git a/lib/diameter/src/base/diameter_service.erl b/lib/diameter/src/base/diameter_service.erl index 76b05a2ad4..f4afa6eb94 100644 --- a/lib/diameter/src/base/diameter_service.erl +++ b/lib/diameter/src/base/diameter_service.erl @@ -1,7 +1,7 @@ %% %% %CopyrightBegin% %% -%% Copyright Ericsson AB 2010-2014. All Rights Reserved. +%% Copyright Ericsson AB 2010-2015. All Rights Reserved. %% %% The contents of this file are subject to the Erlang Public License, %% Version 1.1, (the "License"); you may not use this file except in @@ -610,8 +610,9 @@ st(#watchdog{ref = Ref, pid = Pid}, Refs) -> %% st/3 st(#watchdog{pid = Pid}, Reason, Acc) -> + MRef = monitor(process, Pid), Pid ! {shutdown, self(), Reason}, - [Pid | Acc]. + [MRef | Acc]. %% --------------------------------------------------------------------------- %% # call_service/2 -- cgit v1.2.3 From f21490c2fbaa1b501d2f6d7b766c88b23acc955a Mon Sep 17 00:00:00 2001 From: Anders Svensson Date: Mon, 19 Jan 2015 13:36:12 +0100 Subject: Set shutdown = infinity for supervisor children As suggested in supervisor(3). The leaves of the supervision tree should determine the timeouts. --- lib/diameter/src/base/diameter_sup.erl | 4 ++-- lib/diameter/src/transport/diameter_transport_sup.erl | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/lib/diameter/src/base/diameter_sup.erl b/lib/diameter/src/base/diameter_sup.erl index e5afd23dcd..4ede4086d8 100644 --- a/lib/diameter/src/base/diameter_sup.erl +++ b/lib/diameter/src/base/diameter_sup.erl @@ -1,7 +1,7 @@ %% %% %CopyrightBegin% %% -%% Copyright Ericsson AB 2010-2011. All Rights Reserved. +%% Copyright Ericsson AB 2010-2015. All Rights Reserved. %% %% The contents of this file are subject to the Erlang Public License, %% Version 1.1, (the "License"); you may not use this file except in @@ -64,7 +64,7 @@ spec(Mod) -> {Mod, {Mod, start_link, []}, permanent, - 1000, + infinity, supervisor, [Mod]}. diff --git a/lib/diameter/src/transport/diameter_transport_sup.erl b/lib/diameter/src/transport/diameter_transport_sup.erl index 6457ab78b0..284a41a752 100644 --- a/lib/diameter/src/transport/diameter_transport_sup.erl +++ b/lib/diameter/src/transport/diameter_transport_sup.erl @@ -1,7 +1,7 @@ %% %% %CopyrightBegin% %% -%% Copyright Ericsson AB 2010-2011. All Rights Reserved. +%% Copyright Ericsson AB 2010-2015. All Rights Reserved. %% %% The contents of this file are subject to the Erlang Public License, %% Version 1.1, (the "License"); you may not use this file except in @@ -54,7 +54,7 @@ start_child(Name, Module) -> Spec = {Name, {Module, start_link, [Name]}, permanent, - 1000, + infinity, supervisor, [Module]}, supervisor:start_child(?MODULE, Spec). -- cgit v1.2.3 From 2b1ec3dc6d936f6e885dfe23ea68ed54c98bb950 Mon Sep 17 00:00:00 2001 From: Anders Svensson Date: Mon, 19 Jan 2015 14:16:31 +0100 Subject: Increase service shutdown timeout Shutting down the service causes DPR to be sent on all open transports under the service. These in turn have a timeout for the reception of DPA, but the timeout is bounded by the supervisor's in practice. Both timeouts were 1 second. Increase the supervisor timeout to 5 seconds. Note that the service supervisor is furthest to the right in the supervision tree in diameter_sup. Thus is significant, so that the transport-related processes aren't shutdown first. --- lib/diameter/src/base/diameter_service_sup.erl | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/lib/diameter/src/base/diameter_service_sup.erl b/lib/diameter/src/base/diameter_service_sup.erl index 153fff902f..e3177f0083 100644 --- a/lib/diameter/src/base/diameter_service_sup.erl +++ b/lib/diameter/src/base/diameter_service_sup.erl @@ -1,7 +1,7 @@ %% %% %CopyrightBegin% %% -%% Copyright Ericsson AB 2010-2011. All Rights Reserved. +%% Copyright Ericsson AB 2010-2015. All Rights Reserved. %% %% The contents of this file are subject to the Erlang Public License, %% Version 1.1, (the "License"); you may not use this file except in @@ -58,7 +58,7 @@ init([]) -> ChildSpec = {Mod, {Mod, start_link, []}, temporary, - 1000, + 5000, worker, [Mod]}, {ok, {Flags, [ChildSpec]}}. -- cgit v1.2.3 From bf351d224810e95330c8d317b6d54cc010c5f8f0 Mon Sep 17 00:00:00 2001 From: Anders Svensson Date: Mon, 19 Jan 2015 21:05:41 +0100 Subject: Fix retransmission of messages sent as header/avps list Extracting the End-to-End and Hop-by-Hop identifiers resulted in a function clause error, causing the send to fail. --- lib/diameter/src/base/diameter_codec.erl | 3 +++ lib/diameter/src/base/diameter_traffic.erl | 19 ++++++++++++++++--- 2 files changed, 19 insertions(+), 3 deletions(-) diff --git a/lib/diameter/src/base/diameter_codec.erl b/lib/diameter/src/base/diameter_codec.erl index a2b04bfd63..f9bc44cf61 100644 --- a/lib/diameter/src/base/diameter_codec.erl +++ b/lib/diameter/src/base/diameter_codec.erl @@ -390,6 +390,9 @@ sequence_numbers(#diameter_packet{bin = Bin}) sequence_numbers(#diameter_packet{header = #diameter_header{} = H}) -> sequence_numbers(H); +sequence_numbers(#diameter_packet{msg = [#diameter_header{} = H | _]}) -> + sequence_numbers(H); + sequence_numbers(#diameter_header{hop_by_hop_id = H, end_to_end_id = E}) -> {H,E}; diff --git a/lib/diameter/src/base/diameter_traffic.erl b/lib/diameter/src/base/diameter_traffic.erl index 3b62afca47..d9877bdd8e 100644 --- a/lib/diameter/src/base/diameter_traffic.erl +++ b/lib/diameter/src/base/diameter_traffic.erl @@ -1388,6 +1388,21 @@ make_request_packet(#diameter_packet{header = Hdr} = Pkt, make_request_packet(Msg, Pkt) -> Pkt#diameter_packet{msg = Msg}. +%% make_retransmit_packet/2 + +make_retransmit_packet(#diameter_packet{msg = [#diameter_header{} = Hdr + | Avps]} + = Pkt) -> + Pkt#diameter_packet{msg = [make_retransmit_header(Hdr) | Avps]}; + +make_retransmit_packet(#diameter_packet{header = Hdr} = Pkt) -> + Pkt#diameter_packet{header = make_retransmit_header(Hdr)}. + +%% make_retransmit_header/1 + +make_retransmit_header(Hdr) -> + Hdr#diameter_header{is_retransmitted = true}. + %% fold_record/2 fold_record(undefined, R) -> @@ -1674,9 +1689,7 @@ retransmit({TPid, Caps, App} have_request(Pkt0, TPid) %% Don't failover to a peer we've andalso ?THROW(timeout), %% already sent to. - #diameter_packet{header = Hdr0} = Pkt0, - Hdr = Hdr0#diameter_header{is_retransmitted = true}, - Pkt = Pkt0#diameter_packet{header = Hdr}, + Pkt = make_retransmit_packet(Pkt0), retransmit(cb(App, prepare_retransmit, [Pkt, SvcName, {TPid, Caps}]), Transport, -- cgit v1.2.3 From 3ecaab1ab162cd07dc245d0981454feb1b72265f Mon Sep 17 00:00:00 2001 From: Anders Svensson Date: Wed, 31 Dec 2014 11:29:13 +0100 Subject: Don't discard outgoing answers with Result-Code/E-bit errors Outgoing answers missing a Result-Code AVP or setting an E-bit inappropriately were discarded, but there's no particular reason for doing so if the answer can be encoded, and the sender has no way of knowing that their answer has been discarded. It's also inappropriate that the message be discarded in the relay case. Answers are now sent, and an error counter incremented. --- lib/diameter/src/base/diameter_traffic.erl | 16 ++++++++++------ lib/diameter/test/diameter_traffic_SUITE.erl | 11 ++++++++--- 2 files changed, 18 insertions(+), 9 deletions(-) diff --git a/lib/diameter/src/base/diameter_traffic.erl b/lib/diameter/src/base/diameter_traffic.erl index 3b62afca47..e3ecd9c930 100644 --- a/lib/diameter/src/base/diameter_traffic.erl +++ b/lib/diameter/src/base/diameter_traffic.erl @@ -162,24 +162,28 @@ incr_error(Dir, Id, TPid) -> %% incr_rc/4 %% --------------------------------------------------------------------------- --spec incr_rc(send|recv, Pkt, TPid, Dict0) +-spec incr_rc(send|recv, Pkt, TPid, DictT) -> {Counter, non_neg_integer()} | Reason when Pkt :: #diameter_packet{}, TPid :: pid(), - Dict0 :: module(), + DictT :: module() | {module(), module(), module()}, Counter :: {'Result-Code', integer()} | {'Experimental-Result', integer(), integer()}, Reason :: atom(). -incr_rc(Dir, Pkt, TPid, Dict0) -> +incr_rc(Dir, Pkt, TPid, {Dict, _, _} = DictT) -> try - incr_result(Dir, Pkt, TPid, {Dict0, Dict0, Dict0}) + incr_result(Dir, Pkt, TPid, DictT) catch exit: {E,_} when E == no_result_code; E == invalid_error_bit -> + incr(TPid, {msg_id(Pkt#diameter_packet.header, Dict), Dir, E}), E - end. + end; + +incr_rc(Dir, Pkt, TPid, Dict0) -> + incr_rc(Dir, Pkt, TPid, {Dict0, Dict0, Dict0}). %% --------------------------------------------------------------------------- %% pending/1 @@ -678,7 +682,7 @@ local(Msg, TPid, {Dict, AppDict, Dict0} = DictT, Fs, ReqPkt) -> reset(make_answer_packet(Msg, ReqPkt), Dict, Dict0), Fs), incr(send, Pkt, TPid, AppDict), - incr_result(send, Pkt, TPid, DictT), %% count outgoing + incr_rc(send, Pkt, TPid, DictT), %% count outgoing send(TPid, Pkt). %% reset/3 diff --git a/lib/diameter/test/diameter_traffic_SUITE.erl b/lib/diameter/test/diameter_traffic_SUITE.erl index 4b67372016..92c81d2d18 100644 --- a/lib/diameter/test/diameter_traffic_SUITE.erl +++ b/lib/diameter/test/diameter_traffic_SUITE.erl @@ -414,12 +414,13 @@ send_eval(Config) -> = call(Config, Req). %% Send an accounting ACR that the server tries to answer with an -%% inappropriate header, resulting in no answer being sent and the -%% request timing out. +%% inappropriate header. That the error is detected is coded in +%% handle_answer. send_bad_answer(Config) -> Req = ['ACR', {'Accounting-Record-Type', ?EVENT_RECORD}, {'Accounting-Record-Number', 2}], - {timeout, _} = call(Config, Req). + ?answer_message(?SUCCESS) + = call(Config, Req). %% Send an ACR that the server callback answers explicitly with a %% protocol error. @@ -1057,6 +1058,10 @@ answer(Pkt, Req, _Peer, Name, #group{client_dict0 = Dict0}) -> [R | Vs] = Dict:'#get-'(answer(Ans, Es, Name)), [Dict:rec2msg(R) | Vs]. +answer(Rec, Es, send_bad_answer) -> + [{5004, #diameter_avp{name = 'Result-Code'}} | _] = Es, + Rec; + answer(Rec, [_|_], N) when N == send_long_avp_length; N == send_short_avp_length; -- cgit v1.2.3 From 340e832013daabadd7b85fa6b3e08d34cf17a4a4 Mon Sep 17 00:00:00 2001 From: Anders Svensson Date: Thu, 1 Jan 2015 11:57:00 +0100 Subject: Fix error matching in traffic suite Clause matching error for specific test cases was harmless since the subsequent clause also matched. Errors detected by the server result in Failed-AVP being sent, which should not lead to a decode error in the client. --- lib/diameter/test/diameter_traffic_SUITE.erl | 11 ++--------- 1 file changed, 2 insertions(+), 9 deletions(-) diff --git a/lib/diameter/test/diameter_traffic_SUITE.erl b/lib/diameter/test/diameter_traffic_SUITE.erl index 92c81d2d18..03815f283e 100644 --- a/lib/diameter/test/diameter_traffic_SUITE.erl +++ b/lib/diameter/test/diameter_traffic_SUITE.erl @@ -1058,19 +1058,12 @@ answer(Pkt, Req, _Peer, Name, #group{client_dict0 = Dict0}) -> [R | Vs] = Dict:'#get-'(answer(Ans, Es, Name)), [Dict:rec2msg(R) | Vs]. +%% An inappropriate E-bit results in a decode error ... answer(Rec, Es, send_bad_answer) -> [{5004, #diameter_avp{name = 'Result-Code'}} | _] = Es, Rec; -answer(Rec, [_|_], N) - when N == send_long_avp_length; - N == send_short_avp_length; - N == send_zero_avp_length; - N == send_invalid_avp_length; - N == send_invalid_reject; - N == send_unknown_short_mandatory; - N == send_unexpected_mandatory_decode -> - Rec; +%% ... while other errors are reflected in Failed-AVP. answer(Rec, [], _) -> Rec. -- cgit v1.2.3