From b3d9e0c09ff9d8f963b6084a84ab120cd423a9ae Mon Sep 17 00:00:00 2001 From: Anders Svensson Date: Tue, 25 Jul 2017 01:27:58 +0200 Subject: Redo message decode as a single pass Decode has previously been two passes: first chunk the message into a reversed list of toplevel diameter_avp records, then fold through the reversed list to build the full result. Various workarounds have made it a bit more convoluted than it should be however. Rework it completely, turning the previous 2-pass tail-recursive implementation into a 1-pass body recursive one. The relay decode still exists in diameter_codec, as a stripped down version of the full decode in diameter_gen. --- lib/diameter/include/diameter_gen.hrl | 8 +- lib/diameter/src/base/diameter_codec.erl | 185 +++----- lib/diameter/src/base/diameter_gen.erl | 648 +++++++++++++---------------- lib/diameter/src/base/diameter_traffic.erl | 12 +- 4 files changed, 362 insertions(+), 491 deletions(-) diff --git a/lib/diameter/include/diameter_gen.hrl b/lib/diameter/include/diameter_gen.hrl index fb6370fe54..548763ec7d 100644 --- a/lib/diameter/include/diameter_gen.hrl +++ b/lib/diameter/include/diameter_gen.hrl @@ -26,13 +26,13 @@ %% encode_avps/3 -encode_avps(Name, Vals, Opts) -> - diameter_gen:encode_avps(Name, Vals, Opts#{module => ?MODULE}). +encode_avps(Name, Avps, Opts) -> + diameter_gen:encode_avps(Name, Avps, Opts#{module => ?MODULE}). %% decode_avps/2 -decode_avps(Name, Recs, Opts) -> - diameter_gen:decode_avps(Name, Recs, Opts#{module => ?MODULE}). +decode_avps(Name, Avps, Opts) -> + diameter_gen:decode_avps(Name, Avps, Opts#{module => ?MODULE}). %% avp/5 diff --git a/lib/diameter/src/base/diameter_codec.erl b/lib/diameter/src/base/diameter_codec.erl index 9b21bf4141..c179c4b362 100644 --- a/lib/diameter/src/base/diameter_codec.erl +++ b/lib/diameter/src/base/diameter_codec.erl @@ -110,7 +110,7 @@ encode(Mod, Opts, Msg) -> enc(_, Opts, #diameter_packet{msg = [#diameter_header{} = Hdr | As]} = Pkt) -> - try encode_avps(reorder(As), Opts) of + try encode_avps(As, Opts) of Avps -> Bin = list_to_binary(Avps), Len = 20 + size(Bin), @@ -206,51 +206,12 @@ values(Avps) -> %% Message as a list of #diameter_avp{} ... encode_avps(_, _, [#diameter_avp{} | _] = Avps, Opts) -> - encode_avps(reorder(Avps), Opts); + encode_avps(Avps, Opts); %% ... or as a tuple list or record. encode_avps(Mod, MsgName, Values, Opts) -> Mod:encode_avps(MsgName, Values, Opts). -%% reorder/1 -%% -%% Reorder AVPs for the relay case using the index field of -%% diameter_avp records. Decode populates this field in collect_avps -%% and presents AVPs in reverse order. A relay then sends the reversed -%% list with a Route-Record AVP prepended. The goal here is just to do -%% lists:reverse/1 in Grouped AVPs and the outer list, but only in the -%% case there are indexed AVPs at all, so as not to reverse lists that -%% have been explicilty sent (unindexed, in the desired order) as a -%% diameter_avp list. The effect is the same as lists:keysort/2, but -%% only on the cases we expect, not a general sort. - -reorder(Avps) -> - case reorder(Avps, []) of - false -> - Avps; - Sorted -> - Sorted - end. - -%% reorder/3 - -%% In case someone has reversed the list already. (Not likely.) -reorder([#diameter_avp{index = 0} | _] = Avps, Acc) -> - Avps ++ Acc; - -%% Assume indexed AVPs are in reverse order. -reorder([#diameter_avp{index = N} = A | Avps], Acc) - when is_integer(N) -> - lists:reverse(Avps, [A | Acc]); - -%% An unindexed AVP. -reorder([H | T], Acc) -> - reorder(T, [H | Acc]); - -%% No indexed members. -reorder([], _) -> - false. - %% encode_avps/2 encode_avps(Avps, Opts) -> @@ -327,13 +288,7 @@ decode(Mod, AppMod, Opts, Pkt) -> %% Relay application: just extract the avp's without any decoding of %% their data since we don't know the application in question. decode(?APP_ID_RELAY, _, _, _, #diameter_packet{} = Pkt) -> - case collect_avps(Pkt) of - {E, As} -> - Pkt#diameter_packet{avps = As, - errors = [E]}; - As -> - Pkt#diameter_packet{avps = As} - end; + collect_avps(Pkt); %% Otherwise decode using the dictionary. decode(_, Mod, AppMod, Opts, #diameter_packet{header = Hdr} = Pkt) -> @@ -342,35 +297,31 @@ decode(_, Mod, AppMod, Opts, #diameter_packet{header = Hdr} = Pkt) -> is_error = IsError} = Hdr, - MsgName = if IsError andalso not IsRequest -> + MsgName = if IsError, not IsRequest -> 'answer-message'; true -> Mod:msg_name(CmdCode, IsRequest) end, - decode_avps(MsgName, Mod, AppMod, Opts, Pkt, collect_avps(Pkt)); + decode_avps(MsgName, Mod, AppMod, Opts, Pkt); decode(Id, Mod, AppMod, Opts, Bin) when is_binary(Bin) -> decode(Id, Mod, AppMod, Opts, #diameter_packet{header = decode_header(Bin), bin = Bin}). -%% decode_avps/6 +%% decode_avps/5 -decode_avps(MsgName, Mod, AppMod, Opts, Pkt, {E, Avps}) -> - ?LOG(invalid_avp_length, Pkt#diameter_packet.header), - #diameter_packet{errors = Failed} - = P - = decode_avps(MsgName, Mod, AppMod, Opts, Pkt, Avps), - P#diameter_packet{errors = [E | Failed]}; - -decode_avps('', _, _, _, Pkt, Avps) -> %% unknown message ... - ?LOG(unknown_message, Pkt#diameter_packet.header), - Pkt#diameter_packet{avps = lists:reverse(Avps), - errors = [3001]}; %% DIAMETER_COMMAND_UNSUPPORTED +decode_avps('', _, _, _, #diameter_packet{header = H, %% unknown message + bin = Bin} + = Pkt) -> + ?LOG(unknown_message, H), + Pkt#diameter_packet{avps = collect_avps(Bin), + errors = [3001]}; %% DIAMETER_COMMAND_UNSUPPORTED %% msg = undefined identifies this case. -decode_avps(MsgName, Mod, AppMod, Opts, Pkt, Avps) -> %% ... or not +decode_avps(MsgName, Mod, AppMod, Opts, #diameter_packet{bin = Bin} = Pkt) -> + <<_:20/binary, Avps/binary>> = Bin, {Rec, As, Errors} = Mod:decode_avps(MsgName, Avps, Opts#{dictionary => AppMod, @@ -380,6 +331,8 @@ decode_avps(MsgName, Mod, AppMod, Opts, Pkt, Avps) -> %% ... or not errors = Errors, avps = As}. +%% reformat/3 + reformat(MsgName, Avps, #{decode_format := T}) when T == map; T == list -> @@ -524,24 +477,21 @@ msg_id(<<_:32, Rbit:1, _:7, CmdCode:24, ApplId:32, _/binary>>) -> %%% # collect_avps/1 %%% --------------------------------------------------------------------------- -%% Note that the returned list of AVP's is reversed relative to their -%% order in the binary. Note also that grouped avp's aren't unraveled, -%% only those at the top level. +%% This is only used for the relay decode. Note that grouped avp's +%% aren't unraveled, only those at the top level. --spec collect_avps(#diameter_packet{} | binary()) - -> [Avp] - | {Error, [Avp]} - when Avp :: #diameter_avp{}, - Error :: {5014, #diameter_avp{}}. +-spec collect_avps(#diameter_packet{}) + -> #diameter_packet{}; + (binary()) + -> [#diameter_avp{}]. -collect_avps(#diameter_packet{bin = <<_:20/binary, Avps/binary>>}) -> - collect_avps(Avps, 0, []); +collect_avps(#diameter_packet{bin = Bin} = Pkt) -> + Pkt#diameter_packet{avps = collect_avps(Bin)}; -collect_avps(Bin) - when is_binary(Bin) -> - collect_avps(Bin, 0, []). +collect_avps(<<_:20/binary, Avps/binary>>) -> + collect(Avps, 0). -%% collect_avps/3 +%% collect/2 %% 0 1 2 3 %% 0 1 2 3 4 5 6 7 8 9 0 1 2 3 4 5 6 7 8 9 0 1 2 3 4 5 6 7 8 9 0 1 @@ -555,65 +505,44 @@ collect_avps(Bin) %% | Data ... %% +-+-+-+-+-+-+-+-+ -collect_avps(<>, - N, - Acc) -> - collect_avps(Code, - if 1 == V -> I; 0 == V -> undefined end, - 1 == M, - 1 == P, - Len - 8 - V*4, %% Might be negative, which ensures - ?PAD(Len), %% failure of the Data match below. - Rest, - N, - Acc); - -collect_avps(<<>>, _, Acc) -> - Acc; +collect(<>, N)-> + Vid = if 1 == V -> I; 0 == V -> undefined end, + DataLen = Len - 8 - V*4, %% Might be negative, which ensures + Pad = ?PAD(Len), %% failure of the match below. + MB = 1 == M, + PB = 1 == P, -%% Header is truncated. pack_avp/1 will pad this at encode if sent in -%% a Failed-AVP. -collect_avps(Bin, _, Acc) -> - {{5014, #diameter_avp{data = Bin}}, Acc}. - -%% collect_avps/9 - -%% Duplicate the diameter_avp creation in each branch below to avoid -%% modifying the record, which profiling has shown to be a relatively -%% costly part of building the list. - -collect_avps(Code, VendorId, M, P, Len, Pad, Rest, N, Acc) -> case Rest of - <> -> + <> -> Avp = #diameter_avp{code = Code, - vendor_id = VendorId, - is_mandatory = M, - need_encryption = P, + vendor_id = Vid, + is_mandatory = MB, + need_encryption = PB, data = Data, index = N}, - collect_avps(T, N+1, [Avp | Acc]); + [Avp | collect(T, N+1)]; _ -> %% Length in header points past the end of the message, or - %% doesn't span the header. As stated in the 6733 text - %% above, it's sufficient to return a zero-filled minimal - %% payload if this is a request. Do this (in cases that we - %% know the type) by inducing a decode failure and letting - %% the dictionary's decode (in diameter_gen) deal with it. - %% - %% Note that the extra bit can only occur in the trailing - %% AVP of a message or Grouped AVP, since a faulty AVP - %% Length is otherwise indistinguishable from a correct - %% one here, as we don't know the types of the AVPs being - %% extracted. - Avp = #diameter_avp{code = Code, - vendor_id = VendorId, - is_mandatory = M, - need_encryption = P, - data = {5014, Rest}, - index = N}, - [Avp | Acc] - end. + %% doesn't span the header. Note that an length error can + %% only occur in the trailing AVP of a message or Grouped + %% AVP, since a faulty AVP Length is otherwise + %% indistinguishable from a correct one here, as we don't + %% know the types of the AVPs being extracted. + [#diameter_avp{code = Code, + vendor_id = Vid, + is_mandatory = MB, + need_encryption = PB, + data = {5014, Rest}, + index = N}] + end; +collect(<<>>, _) -> + []; + +%% Header is truncated. pack_avp/1 will pad this at encode if sent in +%% a Failed-AVP. +collect(Bin, _) -> + [#diameter_avp{data = {5014, Bin}}]. %% 3588: %% diff --git a/lib/diameter/src/base/diameter_gen.erl b/lib/diameter/src/base/diameter_gen.erl index 3d0612c294..3688ff7b8f 100644 --- a/lib/diameter/src/base/diameter_gen.erl +++ b/lib/diameter/src/base/diameter_gen.erl @@ -91,7 +91,7 @@ encode(Name, Vals, Opts, Mod) encode(Name, Map, Opts, Mod) when is_map(Map) -> [enc(Name, F, A, V, Opts, Mod) || {F,A} <- Mod:avp_arity(Name), - V <- [maps:get(F, Map, undefined)]]; + V <- [mget(F, Map, undefined)]]; encode(Name, Rec, Opts, Mod) -> [encode(Name, F, V, Opts, Mod) || {F,V} <- Mod:'#get-'(Rec)]. @@ -221,115 +221,168 @@ enc(AvpName, Value, Opts, Mod) -> %% # decode_avps/3 %% --------------------------------------------------------------------------- --spec decode_avps(parent_name(), [#diameter_avp{}], map()) +-spec decode_avps(parent_name(), binary(), map()) -> {parent_record(), [avp()], Failed} when Failed :: [{5000..5999, #diameter_avp{}}]. -decode_avps(Name, Recs, #{module := Mod, decode_format := Fmt} = Opts) -> - {Avps, {Rec, AM, Failed}} - = mapfoldl(fun(T,A) -> decode(Name, Opts, Mod, T, A) end, - {newrec(Fmt, Mod, Name, Opts), #{}, []}, - Recs), - %% AM counts the number of top-level AVPs, which arities/5 then - %% uses when adding 500[59] errors. - Arities = Mod:avp_arity(Name), - {reformat(Name, Rec, Arities, Mod, Opts, Fmt), +decode_avps(Name, Bin, #{module := Mod, decode_format := Fmt} = Opts) -> + Strict = mget(strict_arities, Opts, decode), + [AM, Avps, Failed | Rec] + = decode(Bin, Name, Mod, Fmt, Strict, Opts, 0, #{}), + %% AM counts the number of top-level AVPs, which missing/5 then + %% uses when appending 5005 errors. + {reformat(Name, Rec, Strict, Mod, Fmt), Avps, - Failed ++ arities(Arities, Opts, Mod, AM, Avps)}. + Failed ++ missing(Name, Strict, Mod, Opts, AM)}. %% Append arity errors so that errors are reported in the order %% encountered. Failed-AVP should typically contain the first %% error encountered. -%% mapfoldl/3 -%% -%% Like lists:mapfoldl/3, but don't reverse the list. +%% decode/8 + +decode(<>, + Name, + Mod, + Fmt, + Strict, + Opts0, + Idx, + AM0) -> + Vid = if 1 == V -> I; true -> undefined end, + MB = 1 == M, + PB = 1 == P, + NameT = Mod:avp_name(Code, Vid), %% {AvpName, Type} | 'AVP' + DataLen = Len - 8 - 4*V, %% possibly negative, causing case match to fail + Pad = (4 - (Len rem 4)) rem 4, + + case Rest of + <> -> + Opts = setopts(NameT, Name, MB, Opts0), + %% Not AvpName or else a failed Failed-AVP + %% decode is packed into 'AVP'. + + Avp = #diameter_avp{code = Code, + vendor_id = Vid, + is_mandatory = MB, + need_encryption = PB, + data = Data, + name = name(NameT), + type = type(NameT), + index = Idx}, + + Dec = decode(Data, Name, NameT, Mod, Opts, Avp), %% decode + + AvpName = field(NameT), + Arity = avp_arity(Name, AvpName, Mod, Opts, Avp), + AM = incr(AvpName, Arity, Strict, AM0), %% count + + Acc = decode(T, Name, Mod, Fmt, Strict, Opts, Idx+1, AM),%% recurse + acc(Acc, Dec, Name, AvpName, Arity, Strict, Mod, Opts, AM0); + _ -> + Avp = #diameter_avp{code = Code, + vendor_id = Vid, + is_mandatory = MB, + need_encryption = PB, + data = Rest, + name = name(NameT), + type = type(NameT), + index = Idx}, + [AM0, [Avp], [{5014, Avp}] | newrec(Fmt, Mod, Name, Strict)] + end; + +decode(<<>>, Name, Mod, Fmt, Strict, _, _, AM) -> + [AM, [], [] | newrec(Fmt, Mod, Name, Strict)]; + +decode(Bin, Name, Mod, Fmt, Strict, _, Idx, AM) -> + Avp = #diameter_avp{data = Bin, + index = Idx}, + [AM, [Avp], [{5014, Avp}] | newrec(Fmt, Mod, Name, Strict)]. -mapfoldl(F, Acc, List) -> - mapfoldl(F, Acc, List, []). +%% Data is a truncated header if command_code = undefined, otherwise +%% payload bytes. The former is padded to the length of a header if +%% the AVP reaches an outgoing encode. +%% +%% RFC 6733 says that an AVP returned with 5014 can contain a minimal +%% payload for the AVP's type, but don't always know the type. -mapfoldl(F, Acc0, [T|Rest], List) -> - {B, Acc} = F(T, Acc0), - mapfoldl(F, Acc, Rest, [B|List]); -mapfoldl(_, Acc, [], List) -> - {List, Acc}. +setopts('AVP', _, _, Opts) -> + Opts; -%% 3588/6733: -%% -%% DIAMETER_MISSING_AVP 5005 -%% The request did not contain an AVP that is required by the Command -%% Code definition. If this value is sent in the Result-Code AVP, a -%% Failed-AVP AVP SHOULD be included in the message. The Failed-AVP -%% AVP MUST contain an example of the missing AVP complete with the -%% Vendor-Id if applicable. The value field of the missing AVP -%% should be of correct minimum length and contain zeros. -%% -%% DIAMETER_AVP_OCCURS_TOO_MANY_TIMES 5009 -%% A message was received that included an AVP that appeared more -%% often than permitted in the message definition. The Failed-AVP -%% AVP MUST be included and contain a copy of the first instance of -%% the offending AVP that exceeded the maximum number of occurrences +setopts({_, Type}, Name, M, Opts) -> + set_failed(Name, set_strict(Type, M, Opts)). -%% arities/5 +%% incr/4 -arities(_, #{strict_arities := T}, _, _, _) - when T /= decode -> - []; +incr(F, A, SA, AM) + when F == 'AVP'; + A == ?ANY; + A == 0; + SA /= decode -> + AM; -arities(Arities, Opts, Mod, AM, Avps) -> - [Count, Map | More] - = lists:foldl(fun({N,T}, A) -> more(N, T, Opts, Mod, AM, A) end, - [0, #{}], - Arities), - less(Count, Map, Avps) ++ lists:reverse(More). +incr(AvpName, _, _, AM) -> + maps:update_with(AvpName, fun incr/1, 1, AM). -%% more/6 +%% incr/1 -more(_Name, ?ANY, _Opts, _Mod, _AM, Acc) -> - Acc; +incr(N) -> + N + 1. -more(Name, {Mn, Mx}, Opts, Mod, AM, Acc) -> - more(Name, Mn, Mx, Opts, Mod, AM, Acc); +%% mget/3 +%% +%% Measurably faster than maps:get/3. -more(Name, 1, Opts, Mod, AM, Acc) -> - more(Name, 1, 1, Opts, Mod, AM, Acc). +mget(Key, Map, Def) -> + case Map of + #{Key := V} -> + V; + _ -> + Def + end. -%% more/7 +%% name/1 -more(Name, Mn, Mx, Opts, Mod, AM, Acc) -> - macc(Name, Mn, maps:get(Name, AM, 0), Mx, Opts, Mod, Acc). +name({Name, _}) -> + Name; +name(_) -> + undefined. -%% macc/7 +%% type/1 -macc(Name, Mn, N, _, Opts, Mod, [M, Map | T]) - when N < Mn -> - [M, Map, {5005, empty_avp(Name, Opts, Mod)} | T]; +type({_, Type}) -> + Type; +type(_) -> + undefined. -macc(Name, _, N, Mx, _Opts, _Mod, [M, Map | T]) - when Mx < N -> - K = N - Mx, - [M + K, maps:put(Name, K, Map) | T]; +%% missing/5 -macc(_Name, _, _, _, _Opts, _Mod, Acc) -> - Acc. +missing(Name, decode, Mod, Opts, AM) -> + [{5005, empty_avp(N, Opts, Mod)} || {N,A} <- Mod:avp_arity(Name), + N /= 'AVP', + Mn <- [min_arity(A)], + 0 < Mn, + mget(N, AM, 0) < Mn]; -%% less/3 +missing(_, _, _, _, _) -> + []. -less(0, _, _) -> - []; +%% 3588/6733: +%% +%% DIAMETER_MISSING_AVP 5005 +%% The request did not contain an AVP that is required by the Command +%% Code definition. If this value is sent in the Result-Code AVP, a +%% Failed-AVP AVP SHOULD be included in the message. The Failed-AVP +%% AVP MUST contain an example of the missing AVP complete with the +%% Vendor-Id if applicable. The value field of the missing AVP +%% should be of correct minimum length and contain zeros. -less(N, Map, [#diameter_avp{name = undefined} | Avps]) -> - less(N, Map, Avps); +%% min_arity/1 -less(N, Map, [#diameter_avp{name = Name} = Avp | Avps]) -> - case Map of - #{Name := 0} -> - [{5009, Avp} | less(N-1, Map, Avps)]; - #{Name := M} -> - less(N, maps:put(Name, M-1, Map), Avps); - _ -> - less(N, Map, Avps) - end. +min_arity(1) -> + 1; +min_arity({Mn,_}) -> + Mn. %% empty_avp/3 @@ -356,55 +409,18 @@ empty_avp(Name, Opts, Mod) -> %% specific errors that can be described by this AVP are described in %% the following section. -%% decode/5 - -decode(Name, Opts, Mod, Avp, Acc) -> - #diameter_avp{code = Code, vendor_id = Vid} - = Avp, - N = Mod:avp_name(Code, Vid), - case Opts of - #{strict_arities := T} when T /= decode -> - decode(Name, Opts, Mod, N, ?ANY, Avp, Acc); - _ -> - {Rec, AM, Failed} = Acc, - F = field(N), - A = Mod:avp_arity(Name, F), - decode(Name, Opts, Mod, N, A, Avp, {Rec, - incr(field(F, A), AM), - Failed}) - end. - %% field/1 field({AvpName, _}) -> AvpName; - field(_) -> 'AVP'. -%% field/2 - -field(_, 0) -> - 'AVP'; - -field(F, _) -> - F. - -%% incr/2 - -incr(Key, Map) -> - maps:update_with(Key, fun incr/1, 1, Map). - -%% incr/1 - -incr(N) -> - N + 1. - -%% decode/7 +%% decode/6 %% AVP not in dictionary. -decode(Name, Opts, Mod, 'AVP', Arity, Avp, Acc) -> - decode_AVP(Name, Arity, Avp, Opts, Mod, Acc); +decode(_Data, _Name, 'AVP', _Mod, _Opts, Avp) -> + Avp; %% 6733, 4.4: %% @@ -453,20 +469,9 @@ decode(Name, Opts, Mod, 'AVP', Arity, Avp, Acc) -> %% defined the RFC's "unrecognized", which is slightly stronger than %% "not defined".) -decode(Name, Opts0, Mod, {AvpName, Type}, Arity, Avp, Acc) -> - #diameter_avp{data = Data, is_mandatory = M} - = Avp, - - %% Whether or not to ignore an M-bit on an encapsulated AVP, or on - %% all AVPs with the service_opt() strict_mbit. - Opts1 = set_strict(Type, M, Opts0), - - %% Whether or not we're decoding within Failed-AVP and should - %% ignore decode errors. +decode(Data, Name, {AvpName, Type}, Mod, Opts, Avp) -> #{dictionary := AppMod, failed_avp := Failed} - = Opts - = set_failed(Name, Opts1), %% Not AvpName or else a failed Failed-AVP - %% decode is packed into 'AVP'. + = Opts, %% Reset the dictionary for best-effort decode of Failed-AVP. DecMod = if Failed -> AppMod; @@ -479,103 +484,55 @@ decode(Name, Opts0, Mod, {AvpName, Type}, Arity, Avp, Acc) -> try avp_decode(Data, AvpName, Opts, DecMod, Mod) of {Rec, As} when Type == 'Grouped' -> - A = Avp#diameter_avp{name = AvpName, - value = Rec, - type = Type}, - {[A|As], pack_avp(Name, Arity, A, Opts, Mod, Acc)}; - + A = Avp#diameter_avp{value = Rec}, + [A | As]; V when Type /= 'Grouped' -> - A = Avp#diameter_avp{name = AvpName, - value = V, - type = Type}, - {A, pack_avp(Name, Arity, A, Opts, Mod, Acc)} + Avp#diameter_avp{value = V} catch - throw: {?MODULE, {grouped, Error, ComponentAvps}} -> - decode_error(Name, - Error, - ComponentAvps, - Opts, - Mod, - Avp#diameter_avp{name = AvpName, - data = trim(Avp#diameter_avp.data), - type = Type}, - Acc); - + throw: {?MODULE, T} -> + decode_error(Failed, T, Avp); error: Reason -> - decode_error(Name, - Reason, - Opts, - Mod, - Avp#diameter_avp{name = AvpName, - data = trim(Avp#diameter_avp.data), - type = Type}, - Acc) + decode_error(Failed, Reason, Name, Mod, Opts, Avp) end. -%% avp_decode/5 - -avp_decode(Data, AvpName, Opts, Mod, Mod) -> - Mod:avp(decode, Data, AvpName, Opts); - -avp_decode(Data, AvpName, Opts, Mod, _) -> - Mod:avp(decode, Data, AvpName, Opts, Mod). - -%% trim/1 +%% decode_error/3 %% -%% Remove any extra bit that was added in diameter_codec to induce a -%% 5014 error. - -trim(#diameter_avp{data = Data} = Avp) -> - Avp#diameter_avp{data = trim(Data)}; - -trim({5014, Bin}) -> - Bin; - -trim(Avps) - when is_list(Avps) -> - lists:map(fun trim/1, Avps); - -trim(Avp) -> - Avp. - -%% decode_error/7 - -decode_error(Name, [_|Rec], _, #{failed_avp := true} = Opts, Mod, Avp, Acc) -> - decode_AVP(Name, Avp#diameter_avp{value = Rec}, Opts, Mod, Acc); - -decode_error(Name, _, _, #{failed_avp := true} = Opts, Mod, Avp, Acc) -> - decode_AVP(Name, Avp, Opts, Mod, Acc); +%% Error when decoding a grouped AVP. -decode_error(_, [Error | _], ComponentAvps, _, _, Avp, Acc) -> - decode_error(Error, Avp, Acc, ComponentAvps); +decode_error(true, {Rec, _, _}, Avp) -> + Avp#diameter_avp{value = Rec}; -decode_error(_, Error, ComponentAvps, _, _, Avp, Acc) -> - decode_error(Error, Avp, Acc, ComponentAvps). +decode_error(false, {_, ComponentAvps, [{RC,A} | _]}, Avp) -> + {RC, [Avp | ComponentAvps], Avp#diameter_avp{data = [A]}}. %% decode_error/6 +%% +%% Error when decoding a non-grouped AVP. -decode_error(Name, _Reason, #{failed_avp := true} = Opts, Mod, Avp, Acc) -> - decode_AVP(Name, Avp, Opts, Mod, Acc); +decode_error(true, _, _, _, _, Avp) -> + Avp; -decode_error(Name, Reason, Opts, Mod, Avp, {Rec, AM, Failed}) -> +decode_error(false, Reason, Name, Mod, Opts, Avp) -> Stack = diameter_lib:get_stacktrace(), - AvpName = Avp#diameter_avp.name, diameter_lib:log(decode_error, ?MODULE, ?LINE, - {Reason, Name, AvpName, Mod, Stack}), - {Avp, {Rec, AM, [rc(Reason, Avp, Opts, Mod) | Failed]}}. + {Reason, Name, Avp#diameter_avp.name, Mod, Stack}), + rc(Reason, Avp, Opts, Mod). -%% decode_error/4 +%% avp_decode/5 + +avp_decode(Data, AvpName, Opts, Mod, Mod) -> + Mod:avp(decode, Data, AvpName, Opts); -decode_error({RC, ErrorData}, Avp, {Rec, AM, Failed}, ComponentAvps) -> - E = Avp#diameter_avp{data = [ErrorData]}, - {[Avp | trim(ComponentAvps)], {Rec, AM, [{RC, E} | Failed]}}. +avp_decode(Data, AvpName, Opts, Mod, _) -> + Mod:avp(decode, Data, AvpName, Opts, Mod). %% set_strict/3 - +%% %% Set false as soon as we see a Grouped AVP that doesn't set the %% M-bit, to ignore the M-bit on an encapsulated AVP. + set_strict('Grouped', false = M, #{strict_mbit := true} = Opts) -> Opts#{strict_mbit := M}; set_strict(_, _, Opts) -> @@ -592,86 +549,82 @@ set_failed('Failed-AVP', #{failed_avp := false} = Opts) -> set_failed(_, Opts) -> Opts. -%% decode_AVP/5 -%% -%% Don't know this AVP: see if it can be packed in an 'AVP' field -%% undecoded. Note that the type field is 'undefined' in this case. - -decode_AVP(Name, Avp, #{strict_arities := T} = Opts, Mod, Acc) - when T /= decode -> - decode_AVP(Name, ?ANY, Avp, Opts, Mod, Acc); - -decode_AVP(Name, Avp, Opts, Mod, Acc) -> - decode_AVP(Name, Mod:avp_arity(Name, 'AVP'), Avp, Opts, Mod, Acc). - -%% decode_AVP/6 - -decode_AVP(Name, Arity, Avp, Opts, Mod, Acc) -> - {trim(Avp), pack_AVP(Name, Arity, Avp, Opts, Mod, Acc)}. - -%% rc/2 - -%% diameter_types will raise an error of this form to communicate -%% DIAMETER_INVALID_AVP_LENGTH (5014). A module specified to a -%% @custom_types tag in a dictionary file can also raise an error of -%% this form. -rc({'DIAMETER', 5014 = RC, _}, #diameter_avp{name = AvpName} = A, Opts, Mod) -> - {RC, A#diameter_avp{data = Mod:empty_value(AvpName, Opts)}}; +%% acc/9 + +acc([AM | Acc], As, Name, AvpName, Arity, Strict, Mod, Opts, AM0) -> + [AM | acc1(Acc, As, Name, AvpName, Arity, Strict, Mod, Opts, AM0)]. + +%% acc1/9 + +%% Faulty AVP, not grouped. +acc1(Acc, {_RC, Avp} = E, _, _, _, _, _, _, _) -> + [Avps, Failed | Rec] = Acc, + [[Avp | Avps], [E | Failed] | Rec]; + +%% Faulty component in grouped AVP. +acc1(Acc, {RC, As, Avp}, _, _, _, _, _, _, _) -> + [Avps, Failed | Rec] = Acc, + [[As | Avps], [{RC, Avp} | Failed] | Rec]; + +%% Grouped AVP ... +acc1([Avps | Acc], [Avp|_] = As, Name, AvpName, Arity, Strict, Mod, Opts, AM)-> + [[As|Avps] | acc2(Acc, Avp, Name, AvpName, Arity, Strict, Mod, Opts, AM)]; + +%% ... or not. +acc1([Avps | Acc], Avp, Name, AvpName, Arity, Strict, Mod, Opts, AM) -> + [[Avp|Avps] | acc2(Acc, Avp, Name, AvpName, Arity, Strict, Mod, Opts, AM)]. + +%% acc2/9 + +%% No errors, but nowhere to pack. +acc2(Acc, Avp, _, 'AVP', 0, _, _, _, _) -> + [Failed | Rec] = Acc, + [[{rc(Avp), Avp} | Failed] | Rec]; + +%% No AVP of this name: try to pack as 'AVP'. +acc2(Acc, Avp, Name, _, 0, Strict, Mod, Opts, AM) -> + Arity = pack_arity(Name, Opts, Mod, Avp), + acc2(Acc, Avp, Name, 'AVP', Arity, Strict, Mod, Opts, AM); + +%% Relaxed arities. +acc2(Acc, Avp, _, AvpName, Arity, Strict, Mod, _, _) + when Strict /= decode -> + pack(Arity, AvpName, Avp, Mod, Acc); + +%% No maximum arity. +acc2(Acc, Avp, _, AvpName, {_,'*'} = Arity, _, Mod, _, _) -> + pack(Arity, AvpName, Avp, Mod, Acc); + +%% Or check. +acc2(Acc, Avp, _, AvpName, Arity, _, Mod, _, AM) -> + Count = maps:get(AvpName, AM, 0), + Mx = max_arity(Arity), + if Mx =< Count -> + [Failed | Rec] = Acc, + [[{5009, Avp} | Failed] | Rec]; + true -> + pack(Arity, AvpName, Avp, Mod, Acc) + end. -%% 3588: +%% 3588/6733: %% -%% DIAMETER_INVALID_AVP_VALUE 5004 -%% The request contained an AVP with an invalid value in its data -%% portion. A Diameter message indicating this error MUST include -%% the offending AVPs within a Failed-AVP AVP. -rc(_, Avp, _, _) -> - {5004, Avp}. - -%% pack_avp/6 - -pack_avp(Name, 0, Avp, Opts, Mod, Acc) -> - pack_AVP(Name, Avp, Opts, Mod, Acc); - -pack_avp(_, Arity, #diameter_avp{name = AvpName} = Avp, _Opts, Mod, Acc) -> - pack(Arity, AvpName, Avp, Mod, Acc). - -%% pack_AVP/5 +%% DIAMETER_AVP_OCCURS_TOO_MANY_TIMES 5009 +%% A message was received that included an AVP that appeared more +%% often than permitted in the message definition. The Failed-AVP +%% AVP MUST be included and contain a copy of the first instance of +%% the offending AVP that exceeded the maximum number of occurrences -pack_AVP(Name, Avp, #{strict_arities := T} = Opts, Mod, Acc) - when T /= decode -> - pack_AVP(Name, ?ANY, Avp, Opts, Mod, Acc); +%% max_arity/1 -pack_AVP(Name, Avp, Opts, Mod, Acc) -> - pack_AVP(Name, Mod:avp_arity(Name, 'AVP'), Avp, Opts, Mod, Acc). +max_arity(1) -> + 1; +max_arity({_,Mx}) -> + Mx. -%% pack_AVP/6 +%% rc/1 -%% Length failure was induced because of a header/payload length -%% mismatch. The AVP Length is reset to match the received data if -%% this AVP is encoded in an answer message, since the length is -%% computed. -%% -%% Data is a truncated header if command_code = undefined, otherwise -%% payload bytes. The former is padded to the length of a header if -%% the AVP reaches an outgoing encode in diameter_codec. -%% -%% RFC 6733 says that an AVP returned with 5014 can contain a minimal -%% payload for the AVP's type, but in this case we don't know the -%% type. - -pack_AVP(_, _, #diameter_avp{data = {5014 = RC, Data}} = Avp, _, _, Acc) -> - {Rec, AM, Failed} = Acc, - {Rec, AM, [{RC, Avp#diameter_avp{data = Data}} | Failed]}; - -pack_AVP(Name, Arity, Avp, Opts, Mod, Acc) -> - case pack_AVP(Name, Opts, Arity, Avp) of - false -> - M = Avp#diameter_avp.is_mandatory, - {Rec, AM, Failed} = Acc, - {Rec, AM, [{if M -> 5001; true -> 5008 end, Avp} | Failed]}; - true -> - pack(Arity, 'AVP', Avp, Mod, Acc) - end. +rc(#diameter_avp{is_mandatory = M}) -> + if M -> 5001; true -> 5008 end. %% 3588: %% @@ -686,42 +639,59 @@ pack_AVP(Name, Arity, Avp, Opts, Mod, Acc) -> %% Failed-AVP AVP MUST be included and contain a copy of the %% offending AVP. -%% pack_AVP/4 +%% pack_arity/4 %% Give Failed-AVP special treatment since (1) it'll contain any %% unrecognized mandatory AVP's and (2) the RFC 3588 grammar failed to %% allow for Failed-AVP in an answer-message. -pack_AVP(_, #{strict_arities := T}, _, _) - when T /= decode -> - true; +pack_arity(Name, _, Mod, #diameter_avp{is_mandatory = M, name = AvpName}) + when Name == 'Failed-AVP'; + Name == 'answer-message', AvpName == 'Failed-AVP'; + not M -> + Mod:avp_arity(Name, 'AVP'); +%% Not testing just Name /= 'Failed-AVP' means we're changing the +%% packing of AVPs nested within Failed-AVP, but the point of +%% ignoring errors within Failed-AVP is to decode as much as +%% possible, and failing because a mandatory AVP couldn't be +%% packed into a dedicated field defeats that point. -pack_AVP(_, _, 0, _) -> - false; +pack_arity(Name, #{strict_mbit := Strict, failed_avp := Failed}, Mod, _) + when not Strict; + Failed -> + Mod:avp_arity(Name, 'AVP'); + +pack_arity(_, _, _, _) -> + 0. -pack_AVP(Name, - #{strict_mbit := Strict, - failed_avp := Failed}, - _, - #diameter_avp{is_mandatory = M, - name = AvpName}) -> - - %% Not testing just Name /= 'Failed-AVP' means we're changing the - %% packing of AVPs nested within Failed-AVP, but the point of - %% ignoring errors within Failed-AVP is to decode as much as - %% possible, and failing because a mandatory AVP couldn't be - %% packed into a dedicated field defeats that point. - - Failed == true - orelse Name == 'Failed-AVP' - orelse (Name == 'answer-message' andalso AvpName == 'Failed-AVP') - orelse not M - orelse not Strict. +%% avp_arity/5 + +avp_arity(Name, 'AVP', Mod, Opts, Avp) -> + pack_arity(Name, Opts, Mod, Avp); + +avp_arity(Name, AvpName, Mod, _, _) -> + Mod:avp_arity(Name, AvpName). + +%% rc/4 + +%% Length error communicated from diameter_types or a +%% @custom_types/@codecs module. +rc({'DIAMETER', 5014 = RC, _}, #diameter_avp{name = AvpName} = A, Opts, Mod) -> + {RC, A#diameter_avp{data = Mod:empty_value(AvpName, Opts)}}; + +%% 3588: +%% +%% DIAMETER_INVALID_AVP_VALUE 5004 +%% The request contained an AVP with an invalid value in its data +%% portion. A Diameter message indicating this error MUST include +%% the offending AVPs within a Failed-AVP AVP. +rc(_, Avp, _, _) -> + {5004, Avp}. %% pack/5 -pack(Arity, F, Avp, Mod, {Rec, AM, Failed}) -> - {set(Arity, F, value(F, Avp), Mod, Rec), AM, Failed}. +pack(Arity, F, Avp, Mod, [Failed | Rec]) -> + [Failed | set(Arity, F, value(F, Avp), Mod, Rec)]. %% set/5 @@ -730,7 +700,7 @@ set(_, _, _, _, false = No) -> set(1, F, Value, _, Map) when is_map(Map) -> - maps:put(F, Value, Map); + Map#{F => Value}; set(_, F, V, _, Map) when is_map(Map) -> @@ -755,36 +725,28 @@ value(_, #diameter_avp{value = V}) -> %% # grouped_avp/3 %% --------------------------------------------------------------------------- --spec grouped_avp(decode, avp_name(), binary() | {5014, binary()}, term()) +%% Note that Grouped is the only AVP type that doesn't just return a +%% decoded value, also returning the list of component diameter_avp +%% records. + +-spec grouped_avp(decode, avp_name(), binary(), term()) -> {avp_record(), [avp()]}; (encode, avp_name(), avp_record() | avp_values(), term()) -> iolist() | no_return(). -%% Length error induced by diameter_codec:collect_avps/1: the AVP -%% length in the header was too short (insufficient for the extracted -%% header) or too long (past the end of the message). An empty payload -%% is sufficient according to the RFC text for 5014. -grouped_avp(decode, _Name, {5014 = RC, _Bin}, _) -> - ?THROW({grouped, {RC, []}, []}); - -grouped_avp(decode, Name, Data, Opts) -> - grouped_decode(Name, diameter_codec:collect_avps(Data), Opts); +%% An error in decoding a component AVP throws the first faulty +%% component, which a catch wraps in the Grouped AVP in question. A +%% partially decoded record is only used when ignoring errors in +%% Failed-AVP. +grouped_avp(decode, Name, Bin, Opts) -> + {Rec, Avps, Es} = T = decode_avps(Name, Bin, Opts), + [] == Es orelse ?THROW(T), + {Rec, Avps}; grouped_avp(encode, Name, Data, Opts) -> encode_avps(Name, Data, Opts). -%% grouped_decode/2 -%% -%% Note that Grouped is the only AVP type that doesn't just return a -%% decoded value, also returning the list of component diameter_avp -%% records. - -%% Length error in trailing component AVP. -grouped_decode(_Name, {Error, Acc}, _) -> - {5014, Avp} = Error, - ?THROW({grouped, Error, [Avp | Acc]}); - %% 7.5. Failed-AVP AVP %% In the case where the offending AVP is embedded within a Grouped AVP, @@ -795,15 +757,6 @@ grouped_decode(_Name, {Error, Acc}, _) -> %% to the single offending AVP. This enables the recipient to detect %% the location of the offending AVP when embedded in a group. -%% An error in decoding a component AVP throws the first faulty -%% component, which the catch in d/3 wraps in the Grouped AVP in -%% question. A partially decoded record is only used when ignoring -%% errors in Failed-AVP. -grouped_decode(Name, ComponentAvps, Opts) -> - {Rec, Avps, Es} = decode_avps(Name, ComponentAvps, Opts), - [] == Es orelse ?THROW({grouped, [{_,_} = hd(Es) | Rec], Avps}), - {Rec, Avps}. - %% --------------------------------------------------------------------------- %% # empty_group/2 %% --------------------------------------------------------------------------- @@ -840,7 +793,7 @@ empty(Name, #{module := Mod} = Opts) -> newrec(false = No, _, _, _) -> No; -newrec(record, Mod, Name, #{strict_arities := T}) +newrec(record, Mod, Name, T) when T /= decode -> RecName = Mod:name2rec(Name), Sz = Mod:'#info-'(RecName, size), @@ -859,16 +812,15 @@ newrec(Mod, Name) -> %% reformat/5 -reformat(_, Map, Arities, _Mod, _Opts, list) -> - [{F,V} || {F,_} <- Arities, #{F := V} <- [Map]]; +reformat(Name, Map, _Strict, Mod, list) -> + [{F,V} || {F,_} <- Mod:avp_arity(Name), #{F := V} <- [Map]]; -reformat(Name, Map, Arities, Mod, Opts, record_from_map) -> - SA = maps:get(strict_arities, Opts, decode), +reformat(Name, Map, Strict, Mod, record_from_map) -> RecName = Mod:name2rec(Name), - list_to_tuple([RecName | [maps:get(F, Map, def(A, SA)) - || {F,A} <- Arities]]); + list_to_tuple([RecName | [mget(F, Map, def(A, Strict)) + || {F,A} <- Mod:avp_arity(Name)]]); -reformat(_, Rec, _, _, _, _) -> +reformat(_, Rec, _, _, _) -> Rec. %% def/2 diff --git a/lib/diameter/src/base/diameter_traffic.erl b/lib/diameter/src/base/diameter_traffic.erl index 3463a93568..3194cb4bf1 100644 --- a/lib/diameter/src/base/diameter_traffic.erl +++ b/lib/diameter/src/base/diameter_traffic.erl @@ -327,9 +327,7 @@ recv_request(Ack, %% A request was sent for an application that is not %% supported. RC = 3007, - Es = Pkt#diameter_packet.errors, - DecPkt = Pkt#diameter_packet{avps = collect_avps(Pkt), - errors = [RC | Es]}, + DecPkt = diameter_codec:collect_avps(Pkt), send_answer(answer_message(RC, Dict0, Caps, DecPkt), TPid, Dict0, @@ -345,14 +343,6 @@ recv_request(Ack, decode(Id, Dict, #recvdata{codec = Opts}, Pkt) -> errors(Id, diameter_codec:decode(Id, Dict, Opts, Pkt)). -collect_avps(Pkt) -> - case diameter_codec:collect_avps(Pkt) of - {_Error, Avps} -> - Avps; - Avps -> - Avps - end. - %% send_A/7 send_A([T | Fs], TPid, App, Dict0, RecvData, DecPkt, Caps) -> -- cgit v1.2.3 From 43b84071754a27894a44ffb0f34d4a03157ebeb5 Mon Sep 17 00:00:00 2001 From: Anders Svensson Date: Thu, 3 Aug 2017 14:09:33 +0200 Subject: Don't extract options unnecessarily at encode Extract strict_arities once and pass it through as an argument. --- lib/diameter/src/base/diameter_gen.erl | 88 +++++++++++++++++----------------- 1 file changed, 45 insertions(+), 43 deletions(-) diff --git a/lib/diameter/src/base/diameter_gen.erl b/lib/diameter/src/base/diameter_gen.erl index 3688ff7b8f..4bfad345d0 100644 --- a/lib/diameter/src/base/diameter_gen.erl +++ b/lib/diameter/src/base/diameter_gen.erl @@ -60,8 +60,9 @@ | no_return(). encode_avps(Name, Vals, #{module := Mod} = Opts) -> + Strict = mget(strict_arities, Opts, encode), try - encode(Name, Vals, Opts, Mod) + encode(Name, Vals, Opts, Strict, Mod) catch throw: {?MODULE, Reason} -> diameter_lib:log({encode, error}, @@ -78,87 +79,88 @@ encode_avps(Name, Vals, #{module := Mod} = Opts) -> erlang:error({encode_failure, Reason, Name, Stack}) end. -%% encode/4 - -encode(Name, Vals, #{ordered_encode := false} = Opts, Mod) - when is_list(Vals) -> - lists:map(fun({F,V}) -> encode(Name, F, V, Opts, Mod) end, Vals); +%% encode/5 -encode(Name, Vals, Opts, Mod) +encode(Name, Vals, Opts, Strict, Mod) when is_list(Vals) -> - encode(Name, Mod:'#set-'(Vals, newrec(Mod, Name)), Opts, Mod); + case Opts of + #{ordered_encode := false} -> + lists:map(fun({F,V}) -> encode(Name, F, V, Opts, Strict, Mod) end, + Vals); + _ -> + Rec = Mod:'#set-'(Vals, newrec(Mod, Name)), + encode(Name, Rec, Opts, Strict, Mod) + end; -encode(Name, Map, Opts, Mod) +encode(Name, Map, Opts, Strict, Mod) when is_map(Map) -> - [enc(Name, F, A, V, Opts, Mod) || {F,A} <- Mod:avp_arity(Name), - V <- [mget(F, Map, undefined)]]; + [enc(Name, F, A, V, Opts, Strict, Mod) || {F,A} <- Mod:avp_arity(Name), + V <- [mget(F, Map, undefined)]]; -encode(Name, Rec, Opts, Mod) -> - [encode(Name, F, V, Opts, Mod) || {F,V} <- Mod:'#get-'(Rec)]. +encode(Name, Rec, Opts, Strict, Mod) -> + [encode(Name, F, V, Opts, Strict, Mod) || {F,V} <- Mod:'#get-'(Rec)]. -%% encode/5 +%% encode/6 -encode(Name, AvpName, Values, #{strict_arities := T} = Opts, Mod) - when T /= encode -> - enc(Name, AvpName, ?ANY, Values, Opts, Mod); +encode(Name, AvpName, Values, Opts, Strict, Mod) + when Strict /= encode -> + enc(Name, AvpName, ?ANY, Values, Opts, Strict, Mod); -encode(Name, AvpName, Values, Opts, Mod) -> - enc(Name, AvpName, Mod:avp_arity(Name, AvpName), Values, Opts, Mod). +encode(Name, AvpName, Values, Opts, Strict, Mod) -> + Arity = Mod:avp_arity(Name, AvpName), + enc(Name, AvpName, Arity, Values, Opts, Strict, Mod). -%% enc/6 +%% enc/7 -enc(Name, AvpName, Arity, Values, #{strict_arities := T} = Opts, Mod) - when T /= encode, Arity /= ?ANY -> - enc(Name, AvpName, ?ANY, Values, Opts, Mod); +enc(Name, AvpName, Arity, Values, Opts, Strict, Mod) + when Strict /= encode, Arity /= ?ANY -> + enc(Name, AvpName, ?ANY, Values, Opts, Strict, Mod); -enc(_, AvpName, 1, undefined, _, _) -> +enc(_, AvpName, 1, undefined, _, _, _) -> ?THROW([mandatory_avp_missing, AvpName]); -enc(Name, AvpName, 1, Value, Opts, Mod) -> +enc(Name, AvpName, 1, Value, Opts, _, Mod) -> H = avp_header(AvpName, Mod), enc1(Name, AvpName, H, Value, Opts, Mod); -enc(_, _, {0,_}, [], _, _) -> +enc(_, _, {0,_}, [], _, _, _) -> []; -enc(_, _, _, undefined, _, _) -> +enc(_, _, _, undefined, _, _, _) -> []; %% Be forgiving when a list of values is expected. If the value itself %% is a list then the user has to wrap it to avoid each member from %% being interpreted as an individual AVP value. -enc(Name, AvpName, Arity, V, Opts, Mod) +enc(Name, AvpName, Arity, V, Opts, Strict, Mod) when not is_list(V) -> - enc(Name, AvpName, Arity, [V], Opts, Mod); + enc(Name, AvpName, Arity, [V], Opts, Strict, Mod); -enc(Name, AvpName, {Min, Max}, Values, Opts, Mod) -> +enc(Name, AvpName, {Min, Max}, Values, Opts, Strict, Mod) -> H = avp_header(AvpName, Mod), - enc(Name, AvpName, H, Min, 0, Max, Values, Opts, Mod). + enc(Name, AvpName, H, Min, 0, Max, Values, Opts, Strict, Mod). -%% enc/9 - -enc(Name, AvpName, H, Min, N, '*', Vs, Opts, Mod) - when Min =< N -> - [enc1(Name, AvpName, H, V, Opts, Mod) || V <- Vs]; +%% enc/10 -enc(Name, AvpName, H, _, _, _, Vs, #{strict_arities := T} = Opts, Mod) - when T /= encode -> +enc(Name, AvpName, H, Min, N, Max, Vs, Opts, Strict, Mod) + when Strict /= encode; + Max == '*', Min =< N -> [enc1(Name, AvpName, H, V, Opts, Mod) || V <- Vs]; -enc(_, AvpName, _, Min, N, _, [], _, _) +enc(_, AvpName, _, Min, N, _, [], _, _, _) when N < Min -> ?THROW([repeated_avp_insufficient_arity, AvpName, Min, N]); -enc(_, _, _, _, _, _, [], _, _) -> +enc(_, _, _, _, _, _, [], _, _, _) -> []; -enc(_, AvpName, _, _, N, Max, _, _, _) +enc(_, AvpName, _, _, N, Max, _, _, _, _) when Max =< N -> ?THROW([repeated_avp_excessive_arity, AvpName, Max]); -enc(Name, AvpName, H, Min, N, Max, [V|Vs], Opts, Mod) -> +enc(Name, AvpName, H, Min, N, Max, [V|Vs], Opts, Strict, Mod) -> [enc1(Name, AvpName, H, V, Opts, Mod) - | enc(Name, AvpName, H, Min, N+1, Max, Vs, Opts, Mod)]. + | enc(Name, AvpName, H, Min, N+1, Max, Vs, Opts, Strict, Mod)]. %% avp_header/2 -- cgit v1.2.3 From 96cd627acfde682875149effda4611dc778622fd Mon Sep 17 00:00:00 2001 From: Anders Svensson Date: Sun, 30 Jul 2017 01:27:42 +0200 Subject: Count AVPs in #diameter_avp.index The index field in record diameter_avp was previously used to enumerate AVPs so that the list could be returned in some cases, since diameter_codec:collect_avps/2 (now /1) reversed the order. That's no longer the case as of the grandparent commit, so use the field to enumerate instances of the same AVP instead, and only when arities are being checked, to save having to look them up in the map when checking for 5009 errors, or counting AVPs at all in diameter_codec:collect_avps/1. --- lib/diameter/src/base/diameter_codec.erl | 18 +++--- lib/diameter/src/base/diameter_gen.erl | 104 +++++++++++++++---------------- 2 files changed, 58 insertions(+), 64 deletions(-) diff --git a/lib/diameter/src/base/diameter_codec.erl b/lib/diameter/src/base/diameter_codec.erl index c179c4b362..81c21fb8f2 100644 --- a/lib/diameter/src/base/diameter_codec.erl +++ b/lib/diameter/src/base/diameter_codec.erl @@ -489,9 +489,9 @@ collect_avps(#diameter_packet{bin = Bin} = Pkt) -> Pkt#diameter_packet{avps = collect_avps(Bin)}; collect_avps(<<_:20/binary, Avps/binary>>) -> - collect(Avps, 0). + collect(Avps). -%% collect/2 +%% collect/1 %% 0 1 2 3 %% 0 1 2 3 4 5 6 7 8 9 0 1 2 3 4 5 6 7 8 9 0 1 2 3 4 5 6 7 8 9 0 1 @@ -505,7 +505,7 @@ collect_avps(<<_:20/binary, Avps/binary>>) -> %% | Data ... %% +-+-+-+-+-+-+-+-+ -collect(<>, N)-> +collect(<>) -> Vid = if 1 == V -> I; 0 == V -> undefined end, DataLen = Len - 8 - V*4, %% Might be negative, which ensures Pad = ?PAD(Len), %% failure of the match below. @@ -518,9 +518,8 @@ collect(<>, N)-> vendor_id = Vid, is_mandatory = MB, need_encryption = PB, - data = Data, - index = N}, - [Avp | collect(T, N+1)]; + data = Data}, + [Avp | collect(T)]; _ -> %% Length in header points past the end of the message, or %% doesn't span the header. Note that an length error can @@ -532,16 +531,15 @@ collect(<>, N)-> vendor_id = Vid, is_mandatory = MB, need_encryption = PB, - data = {5014, Rest}, - index = N}] + data = {5014, Rest}}] end; -collect(<<>>, _) -> +collect(<<>>) -> []; %% Header is truncated. pack_avp/1 will pad this at encode if sent in %% a Failed-AVP. -collect(Bin, _) -> +collect(Bin) -> [#diameter_avp{data = {5014, Bin}}]. %% 3588: diff --git a/lib/diameter/src/base/diameter_gen.erl b/lib/diameter/src/base/diameter_gen.erl index 4bfad345d0..f0196ccaa9 100644 --- a/lib/diameter/src/base/diameter_gen.erl +++ b/lib/diameter/src/base/diameter_gen.erl @@ -230,7 +230,7 @@ enc(AvpName, Value, Opts, Mod) -> decode_avps(Name, Bin, #{module := Mod, decode_format := Fmt} = Opts) -> Strict = mget(strict_arities, Opts, decode), [AM, Avps, Failed | Rec] - = decode(Bin, Name, Mod, Fmt, Strict, Opts, 0, #{}), + = decode(Bin, Name, Mod, Fmt, Strict, Opts, #{}), %% AM counts the number of top-level AVPs, which missing/5 then %% uses when appending 5005 errors. {reformat(Name, Rec, Strict, Mod, Fmt), @@ -241,7 +241,7 @@ decode_avps(Name, Bin, #{module := Mod, decode_format := Fmt} = Opts) -> %% encountered. Failed-AVP should typically contain the first %% error encountered. -%% decode/8 +%% decode/7 decode(<>, Name, @@ -249,7 +249,6 @@ decode(<>, Fmt, Strict, Opts0, - Idx, AM0) -> Vid = if 1 == V -> I; true -> undefined end, MB = 1 == M, @@ -258,12 +257,16 @@ decode(<>, DataLen = Len - 8 - 4*V, %% possibly negative, causing case match to fail Pad = (4 - (Len rem 4)) rem 4, + Opts = setopts(NameT, Name, MB, Opts0), + %% Not AvpName or else a failed Failed-AVP + %% decode is packed into 'AVP'. + + AvpName = field(NameT), + Arity = avp_arity(Name, AvpName, Mod, Opts, MB), + {Idx, AM} = incr(AvpName, Arity, Strict, AM0), %% count + case Rest of <> -> - Opts = setopts(NameT, Name, MB, Opts0), - %% Not AvpName or else a failed Failed-AVP - %% decode is packed into 'AVP'. - Avp = #diameter_avp{code = Code, vendor_id = Vid, is_mandatory = MB, @@ -272,15 +275,9 @@ decode(<>, name = name(NameT), type = type(NameT), index = Idx}, - - Dec = decode(Data, Name, NameT, Mod, Opts, Avp), %% decode - - AvpName = field(NameT), - Arity = avp_arity(Name, AvpName, Mod, Opts, Avp), - AM = incr(AvpName, Arity, Strict, AM0), %% count - - Acc = decode(T, Name, Mod, Fmt, Strict, Opts, Idx+1, AM),%% recurse - acc(Acc, Dec, Name, AvpName, Arity, Strict, Mod, Opts, AM0); + Dec = decode(Data, Name, NameT, Mod, Opts, Avp), %% decode + Acc = decode(T, Name, Mod, Fmt, Strict, Opts, AM), %% recurse + acc(Acc, Dec, Name, AvpName, Arity, Strict, Mod, Opts); _ -> Avp = #diameter_avp{code = Code, vendor_id = Vid, @@ -290,15 +287,14 @@ decode(<>, name = name(NameT), type = type(NameT), index = Idx}, - [AM0, [Avp], [{5014, Avp}] | newrec(Fmt, Mod, Name, Strict)] + [AM, [Avp], [{5014, Avp}] | newrec(Fmt, Mod, Name, Strict)] end; -decode(<<>>, Name, Mod, Fmt, Strict, _, _, AM) -> +decode(<<>>, Name, Mod, Fmt, Strict, _, AM) -> [AM, [], [] | newrec(Fmt, Mod, Name, Strict)]; -decode(Bin, Name, Mod, Fmt, Strict, _, Idx, AM) -> - Avp = #diameter_avp{data = Bin, - index = Idx}, +decode(Bin, Name, Mod, Fmt, Strict, _, AM) -> + Avp = #diameter_avp{data = Bin}, [AM, [Avp], [{5014, Avp}] | newrec(Fmt, Mod, Name, Strict)]. %% Data is a truncated header if command_code = undefined, otherwise @@ -321,15 +317,15 @@ incr(F, A, SA, AM) A == ?ANY; A == 0; SA /= decode -> - AM; + {undefined, AM}; incr(AvpName, _, _, AM) -> - maps:update_with(AvpName, fun incr/1, 1, AM). - -%% incr/1 - -incr(N) -> - N + 1. + case AM of + #{AvpName := N} -> + {N, AM#{AvpName => N+1}}; + _ -> + {0, AM#{AvpName => 1}} + end. %% mget/3 %% @@ -551,57 +547,57 @@ set_failed('Failed-AVP', #{failed_avp := false} = Opts) -> set_failed(_, Opts) -> Opts. -%% acc/9 +%% acc/8 -acc([AM | Acc], As, Name, AvpName, Arity, Strict, Mod, Opts, AM0) -> - [AM | acc1(Acc, As, Name, AvpName, Arity, Strict, Mod, Opts, AM0)]. +acc([AM | Acc], As, Name, AvpName, Arity, Strict, Mod, Opts) -> + [AM | acc1(Acc, As, Name, AvpName, Arity, Strict, Mod, Opts)]. -%% acc1/9 +%% acc1/8 %% Faulty AVP, not grouped. -acc1(Acc, {_RC, Avp} = E, _, _, _, _, _, _, _) -> +acc1(Acc, {_RC, Avp} = E, _, _, _, _, _, _) -> [Avps, Failed | Rec] = Acc, [[Avp | Avps], [E | Failed] | Rec]; %% Faulty component in grouped AVP. -acc1(Acc, {RC, As, Avp}, _, _, _, _, _, _, _) -> +acc1(Acc, {RC, As, Avp}, _, _, _, _, _, _) -> [Avps, Failed | Rec] = Acc, [[As | Avps], [{RC, Avp} | Failed] | Rec]; %% Grouped AVP ... -acc1([Avps | Acc], [Avp|_] = As, Name, AvpName, Arity, Strict, Mod, Opts, AM)-> - [[As|Avps] | acc2(Acc, Avp, Name, AvpName, Arity, Strict, Mod, Opts, AM)]; +acc1([Avps | Acc], [Avp|_] = As, Name, AvpName, Arity, Strict, Mod, Opts) -> + [[As|Avps] | acc2(Acc, Avp, Name, AvpName, Arity, Strict, Mod, Opts)]; %% ... or not. -acc1([Avps | Acc], Avp, Name, AvpName, Arity, Strict, Mod, Opts, AM) -> - [[Avp|Avps] | acc2(Acc, Avp, Name, AvpName, Arity, Strict, Mod, Opts, AM)]. +acc1([Avps | Acc], Avp, Name, AvpName, Arity, Strict, Mod, Opts) -> + [[Avp|Avps] | acc2(Acc, Avp, Name, AvpName, Arity, Strict, Mod, Opts)]. -%% acc2/9 +%% acc2/8 %% No errors, but nowhere to pack. -acc2(Acc, Avp, _, 'AVP', 0, _, _, _, _) -> +acc2(Acc, Avp, _, 'AVP', 0, _, _, _) -> [Failed | Rec] = Acc, [[{rc(Avp), Avp} | Failed] | Rec]; %% No AVP of this name: try to pack as 'AVP'. -acc2(Acc, Avp, Name, _, 0, Strict, Mod, Opts, AM) -> - Arity = pack_arity(Name, Opts, Mod, Avp), - acc2(Acc, Avp, Name, 'AVP', Arity, Strict, Mod, Opts, AM); +acc2(Acc, Avp, Name, AvpName, 0, Strict, Mod, Opts) -> + M = Avp#diameter_avp.is_mandatory, + Arity = pack_arity(Name, AvpName, Opts, Mod, M), + acc2(Acc, Avp, Name, 'AVP', Arity, Strict, Mod, Opts); %% Relaxed arities. -acc2(Acc, Avp, _, AvpName, Arity, Strict, Mod, _, _) +acc2(Acc, Avp, _, AvpName, Arity, Strict, Mod, _) when Strict /= decode -> pack(Arity, AvpName, Avp, Mod, Acc); %% No maximum arity. -acc2(Acc, Avp, _, AvpName, {_,'*'} = Arity, _, Mod, _, _) -> +acc2(Acc, Avp, _, AvpName, {_,'*'} = Arity, _, Mod, _) -> pack(Arity, AvpName, Avp, Mod, Acc); %% Or check. -acc2(Acc, Avp, _, AvpName, Arity, _, Mod, _, AM) -> - Count = maps:get(AvpName, AM, 0), +acc2(Acc, Avp, _, AvpName, Arity, _, Mod, _) -> Mx = max_arity(Arity), - if Mx =< Count -> + if Mx =< Avp#diameter_avp.index -> [Failed | Rec] = Acc, [[{5009, Avp} | Failed] | Rec]; true -> @@ -641,13 +637,13 @@ rc(#diameter_avp{is_mandatory = M}) -> %% Failed-AVP AVP MUST be included and contain a copy of the %% offending AVP. -%% pack_arity/4 +%% pack_arity/5 %% Give Failed-AVP special treatment since (1) it'll contain any %% unrecognized mandatory AVP's and (2) the RFC 3588 grammar failed to %% allow for Failed-AVP in an answer-message. -pack_arity(Name, _, Mod, #diameter_avp{is_mandatory = M, name = AvpName}) +pack_arity(Name, AvpName, _, Mod, M) when Name == 'Failed-AVP'; Name == 'answer-message', AvpName == 'Failed-AVP'; not M -> @@ -658,18 +654,18 @@ pack_arity(Name, _, Mod, #diameter_avp{is_mandatory = M, name = AvpName}) %% possible, and failing because a mandatory AVP couldn't be %% packed into a dedicated field defeats that point. -pack_arity(Name, #{strict_mbit := Strict, failed_avp := Failed}, Mod, _) +pack_arity(Name, _, #{strict_mbit := Strict, failed_avp := Failed}, Mod, _) when not Strict; Failed -> Mod:avp_arity(Name, 'AVP'); -pack_arity(_, _, _, _) -> +pack_arity(_, _, _, _, _) -> 0. %% avp_arity/5 -avp_arity(Name, 'AVP', Mod, Opts, Avp) -> - pack_arity(Name, Opts, Mod, Avp); +avp_arity(Name, 'AVP' = AvpName, Mod, Opts, M) -> + pack_arity(Name, AvpName, Opts, Mod, M); avp_arity(Name, AvpName, Mod, _, _) -> Mod:avp_arity(Name, AvpName). -- cgit v1.2.3 From 1a2c87f0035849fc5a24bff5dd36796500d1f451 Mon Sep 17 00:00:00 2001 From: Anders Svensson Date: Sat, 29 Jul 2017 16:53:18 +0200 Subject: Optimize sub-binaries With ERL_COMPILER_OPTIONS=bin_opt_info, before: base/diameter_codec.erl:508: Warning: NOT OPTIMIZED: the binary matching instruction that follows in the same function have problems that prevent delayed sub binary optimization (probably indicated by INFO warnings) And after: base/diameter_codec.erl:508: Warning: OPTIMIZED: creation of sub binary delayed This has a surprisingly large impact on the performance of diameter_codec:collect_avps/2: about 15% faster in one testcase on a RAR with 7 AVPs. --- lib/diameter/src/base/diameter_codec.erl | 51 ++++++++++++++++++-------------- 1 file changed, 28 insertions(+), 23 deletions(-) diff --git a/lib/diameter/src/base/diameter_codec.erl b/lib/diameter/src/base/diameter_codec.erl index 81c21fb8f2..c171ea9dca 100644 --- a/lib/diameter/src/base/diameter_codec.erl +++ b/lib/diameter/src/base/diameter_codec.erl @@ -506,20 +506,33 @@ collect_avps(<<_:20/binary, Avps/binary>>) -> %% +-+-+-+-+-+-+-+-+ collect(<>) -> - Vid = if 1 == V -> I; 0 == V -> undefined end, - DataLen = Len - 8 - V*4, %% Might be negative, which ensures - Pad = ?PAD(Len), %% failure of the match below. - MB = 1 == M, - PB = 1 == P, - - case Rest of - <> -> + collect(Rest, + Code, + if 1 == V -> I; 0 == V -> undefined end, + Len - 8 - V*4, %% Might be negative, which ensures + ?PAD(Len), %% failure of the match below. + 1 == M, + 1 == P); + +collect(<<>>) -> + []; + +%% Header is truncated. pack_avp/1 will pad this at encode if sent in +%% a Failed-AVP. +collect(Bin) -> + [#diameter_avp{data = {5014, Bin}}]. + +%% collect/7 + +collect(Bin, Code, Vid, DataLen, Pad, M, P) -> + case Bin of + <> -> Avp = #diameter_avp{code = Code, vendor_id = Vid, - is_mandatory = MB, - need_encryption = PB, + is_mandatory = M, + need_encryption = P, data = Data}, - [Avp | collect(T)]; + [Avp | collect(Rest)]; _ -> %% Length in header points past the end of the message, or %% doesn't span the header. Note that an length error can @@ -529,18 +542,10 @@ collect(<>) -> %% know the types of the AVPs being extracted. [#diameter_avp{code = Code, vendor_id = Vid, - is_mandatory = MB, - need_encryption = PB, - data = {5014, Rest}}] - end; - -collect(<<>>) -> - []; - -%% Header is truncated. pack_avp/1 will pad this at encode if sent in -%% a Failed-AVP. -collect(Bin) -> - [#diameter_avp{data = {5014, Bin}}]. + is_mandatory = M, + need_encryption = P, + data = {5014, Bin}}] + end. %% 3588: %% -- cgit v1.2.3 From bacca97210d31865c46b759115039b22f9ba9ed7 Mon Sep 17 00:00:00 2001 From: Anders Svensson Date: Sat, 29 Jul 2017 17:24:30 +0200 Subject: Optimize sub-binaries Also inline incr/8 and associated functions that were needed for the compiler to accept the optimization, since this does make for a measuable improvement. --- lib/diameter/src/base/diameter_gen.erl | 82 +++++++++++++++++++++++----------- 1 file changed, 56 insertions(+), 26 deletions(-) diff --git a/lib/diameter/src/base/diameter_gen.erl b/lib/diameter/src/base/diameter_gen.erl index f0196ccaa9..7a1a46ec52 100644 --- a/lib/diameter/src/base/diameter_gen.erl +++ b/lib/diameter/src/base/diameter_gen.erl @@ -26,6 +26,14 @@ -module(diameter_gen). +-compile({inline, [incr/8, + incr/4, + field/1, + setopts/4, + avp_arity/5, + set_failed/2, + set_strict/3]}). + -export([encode_avps/3, decode_avps/3, grouped_avp/4, @@ -248,54 +256,76 @@ decode(<>, Mod, Fmt, Strict, - Opts0, - AM0) -> - Vid = if 1 == V -> I; true -> undefined end, - MB = 1 == M, - PB = 1 == P, - NameT = Mod:avp_name(Code, Vid), %% {AvpName, Type} | 'AVP' - DataLen = Len - 8 - 4*V, %% possibly negative, causing case match to fail - Pad = (4 - (Len rem 4)) rem 4, + Opts, + AM) -> + decode(Rest, + Code, + if 1 == V -> I; true -> undefined end, + Len - 8 - 4*V, %% possibly negative, causing case match to fail + (4 - (Len rem 4)) rem 4, + 1 == M, + 1 == P, + Name, + Mod, + Fmt, + Strict, + Opts, + AM); - Opts = setopts(NameT, Name, MB, Opts0), - %% Not AvpName or else a failed Failed-AVP - %% decode is packed into 'AVP'. +decode(<<>>, Name, Mod, Fmt, Strict, _, AM) -> + [AM, [], [] | newrec(Fmt, Mod, Name, Strict)]; - AvpName = field(NameT), - Arity = avp_arity(Name, AvpName, Mod, Opts, MB), - {Idx, AM} = incr(AvpName, Arity, Strict, AM0), %% count +decode(Bin, Name, Mod, Fmt, Strict, _, AM) -> + Avp = #diameter_avp{data = Bin}, + [AM, [Avp], [{5014, Avp}] | newrec(Fmt, Mod, Name, Strict)]. + +%% decode/13 - case Rest of +decode(Bin, Code, Vid, DataLen, Pad, M, P, Name, Mod, Fmt, Strict, Opts0, AM0) -> + case Bin of <> -> + {NameT, AvpName, Arity, {Idx, AM}} + = incr(Name, Code, Vid, M, Mod, Strict, Opts0, AM0), + + Opts = setopts(NameT, Name, M, Opts0), + %% Not AvpName or else a failed Failed-AVP + %% decode is packed into 'AVP'. + Avp = #diameter_avp{code = Code, vendor_id = Vid, - is_mandatory = MB, - need_encryption = PB, + is_mandatory = M, + need_encryption = P, data = Data, name = name(NameT), type = type(NameT), index = Idx}, + Dec = decode(Data, Name, NameT, Mod, Opts, Avp), %% decode Acc = decode(T, Name, Mod, Fmt, Strict, Opts, AM), %% recurse acc(Acc, Dec, Name, AvpName, Arity, Strict, Mod, Opts); _ -> + {NameT, _AvpName, _Arity, {Idx, AM}} + = incr(Name, Code, Vid, M, Mod, Strict, Opts0, AM0), + Avp = #diameter_avp{code = Code, vendor_id = Vid, - is_mandatory = MB, - need_encryption = PB, - data = Rest, + is_mandatory = M, + need_encryption = P, + data = Bin, name = name(NameT), type = type(NameT), index = Idx}, + [AM, [Avp], [{5014, Avp}] | newrec(Fmt, Mod, Name, Strict)] - end; + end. -decode(<<>>, Name, Mod, Fmt, Strict, _, AM) -> - [AM, [], [] | newrec(Fmt, Mod, Name, Strict)]; +%% incr/8 -decode(Bin, Name, Mod, Fmt, Strict, _, AM) -> - Avp = #diameter_avp{data = Bin}, - [AM, [Avp], [{5014, Avp}] | newrec(Fmt, Mod, Name, Strict)]. +incr(Name, Code, Vid, M, Mod, Strict, Opts, AM0) -> + NameT = Mod:avp_name(Code, Vid), %% {AvpName, Type} | 'AVP' + AvpName = field(NameT), + Arity = avp_arity(Name, AvpName, Mod, Opts, M), + {NameT, AvpName, Arity, incr(AvpName, Arity, Strict, AM0)}. %% Data is a truncated header if command_code = undefined, otherwise %% payload bytes. The former is padded to the length of a header if -- cgit v1.2.3 From 5beaaf0cdc856c1a7623a1beee4da74aa9c1825e Mon Sep 17 00:00:00 2001 From: Anders Svensson Date: Thu, 20 Jul 2017 22:58:53 +0200 Subject: Fix type spec That dialyzer hasn't noticed is broken. --- lib/diameter/src/base/diameter_traffic.erl | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/lib/diameter/src/base/diameter_traffic.erl b/lib/diameter/src/base/diameter_traffic.erl index 3194cb4bf1..cfa857e09a 100644 --- a/lib/diameter/src/base/diameter_traffic.erl +++ b/lib/diameter/src/base/diameter_traffic.erl @@ -220,13 +220,13 @@ incr_rc(Dir, Pkt, TPid, Dict0) -> -> pid() %% request handler | boolean() %% answer, known request or not | discard %% request discarded by MFA - when Route :: {Handler, RequestRef, Seqs} + when Route :: {Handler, RequestRef, TPid} | Ack, RecvData :: {[SpawnOpt], #recvdata{}}, SpawnOpt :: term(), Handler :: pid(), RequestRef :: reference(), - Seqs :: {0..16#FFFFFFFF, 0..16#FFFFFFFF}, + TPid :: pid(), Ack :: boolean(). receive_message(TPid, Route, Pkt, Dict0, RecvData) -> -- cgit v1.2.3 From c319fb83523f6a70b2e6c38ad17056bae0ff62f4 Mon Sep 17 00:00:00 2001 From: Anders Svensson Date: Thu, 3 Aug 2017 19:21:47 +0200 Subject: Fix ct return value in traffic suite Which has had no negative effect. --- lib/diameter/test/diameter_traffic_SUITE.erl | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/lib/diameter/test/diameter_traffic_SUITE.erl b/lib/diameter/test/diameter_traffic_SUITE.erl index 662d95e3ae..0dbd363dc6 100644 --- a/lib/diameter/test/diameter_traffic_SUITE.erl +++ b/lib/diameter/test/diameter_traffic_SUITE.erl @@ -317,7 +317,8 @@ init_per_group(Name, Config) when Name == shuffle; Name == parallel -> start_services(Config), - add_transports(Config); + add_transports(Config), + Config; init_per_group(sctp = Name, Config) -> {_, Sctp} = lists:keyfind(Name, 1, Config), -- cgit v1.2.3 From 6e20cd446428685e975a2e51a009c07b1ea2066a Mon Sep 17 00:00:00 2001 From: Anders Svensson Date: Thu, 3 Aug 2017 19:26:02 +0200 Subject: Sleep randomly at the start of (parallel) traffic testcases --- lib/diameter/test/diameter_traffic_SUITE.erl | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/lib/diameter/test/diameter_traffic_SUITE.erl b/lib/diameter/test/diameter_traffic_SUITE.erl index 0dbd363dc6..1760d7c5dc 100644 --- a/lib/diameter/test/diameter_traffic_SUITE.erl +++ b/lib/diameter/test/diameter_traffic_SUITE.erl @@ -318,7 +318,7 @@ init_per_group(Name, Config) Name == parallel -> start_services(Config), add_transports(Config), - Config; + [{sleep, Name == parallel} | Config]; init_per_group(sctp = Name, Config) -> {_, Sctp} = lists:keyfind(Name, 1, Config), @@ -380,6 +380,8 @@ init_per_testcase(Name, Config) -> _ when not Run -> {skip, random}; _ -> + proplists:get_value(sleep, Config, false) + andalso timer:sleep(rand:uniform(200)), [{testcase, Name} | Config] end. -- cgit v1.2.3 From b435807b3aec896493cf3f2fc029c6c12b80ed55 Mon Sep 17 00:00:00 2001 From: Anders Svensson Date: Sun, 23 Jul 2017 13:10:06 +0200 Subject: Restructure/simplify message reception in diameter_peer_fsm Create less garbage. --- lib/diameter/src/base/diameter_peer_fsm.erl | 153 ++++++++++++++-------------- 1 file changed, 74 insertions(+), 79 deletions(-) diff --git a/lib/diameter/src/base/diameter_peer_fsm.erl b/lib/diameter/src/base/diameter_peer_fsm.erl index 36014df94e..4870b86be5 100644 --- a/lib/diameter/src/base/diameter_peer_fsm.erl +++ b/lib/diameter/src/base/diameter_peer_fsm.erl @@ -653,10 +653,6 @@ encode(Rec, Opts, Dict) -> %% incoming/2 -incoming({recv = T, Name, Pkt}, #state{parent = Pid, ack = Ack} = S) -> - Pid ! {T, self(), get_route(Ack, Pkt), Name, Pkt}, - rcv(Name, Pkt, S); - incoming(#diameter_header{is_request = R}, #state{transport = TPid, ack = Ack}) -> R andalso Ack andalso send(TPid, false), @@ -674,98 +670,97 @@ incoming(T, _) -> %% recv/2 -recv(#diameter_packet{header = #diameter_header{} = Hdr} - = Pkt, - #state{dictionary = Dict0} - = S) -> - recv1(diameter_codec:msg_name(Dict0, Hdr), Pkt, S); - -recv(#diameter_packet{header = undefined, - bin = Bin} - = Pkt, - S) -> - recv(diameter_codec:decode_header(Bin), Pkt, S); +recv(#diameter_packet{bin = Bin} = Pkt, S) -> + recv(Bin, Pkt, S); recv(Bin, S) -> - recv(#diameter_packet{bin = Bin}, S). + recv(Bin, Bin, S). + +%% recv/3 + +recv(Bin, Msg, S) -> + recv(diameter_codec:decode_header(Bin), Bin, Msg, S). + +%% recv/4 -%% recv1/3 +recv(false, Bin, _, #state{length_errors = E}) -> + invalid(E, truncated_header, Bin), + Bin; + +recv(#diameter_header{length = Len} = H, Bin, Msg, #state{length_errors = E, + incoming_maxlen = M, + dictionary = Dict0} + = S) + when E == handle; + 0 == Len rem 4, bit_size(Bin) == 8*Len, size(Bin) =< M -> + recv1(diameter_codec:msg_name(Dict0, H), H, Msg, S); -recv1(_, - #diameter_packet{header = H, bin = Bin}, - #state{incoming_maxlen = M}) +recv(H, Bin, _, #state{incoming_maxlen = M}) when M < size(Bin) -> invalid(false, incoming_maxlen_exceeded, {size(Bin), H}), H; +recv(H, Bin, _, #state{length_errors = E}) -> + T = {size(Bin), bit_size(Bin) rem 8, H}, + invalid(E, message_length_mismatch, T), + H. + +%% recv1/4 + %% Ignore anything but an expected CER/CEA if so configured. This is %% non-standard behaviour. -recv1(Name, #diameter_packet{header = H}, #state{state = {'Wait-CEA', _, _}, - strict = false}) +recv1(Name, H, _, #state{state = {'Wait-CEA', _, _}, + strict = false}) when Name /= 'CEA' -> H; -recv1(Name, #diameter_packet{header = H}, #state{state = recv_CER, - strict = false}) +recv1(Name, H, _, #state{state = recv_CER, + strict = false}) when Name /= 'CER' -> H; %% Incoming request after outgoing DPR: discard. Don't discard DPR, so %% both ends don't do so when sending simultaneously. -recv1(Name, - #diameter_packet{header = #diameter_header{is_request = true} = H}, - #state{dpr = {_,_,_}}) +recv1(Name, #diameter_header{is_request = true} = H, _, #state{dpr = {_,_,_}}) when Name /= 'DPR' -> invalid(false, recv_after_outgoing_dpr, H), H; %% Incoming request after incoming DPR: discard. -recv1(_, - #diameter_packet{header = #diameter_header{is_request = true} = H}, - #state{dpr = true}) -> +recv1(_, #diameter_header{is_request = true} = H, _, #state{dpr = true}) -> invalid(false, recv_after_incoming_dpr, H), H; %% DPA with identifier mismatch, or in response to a DPR initiated by %% the service. -recv1('DPA' = N, - #diameter_packet{header = #diameter_header{hop_by_hop_id = Hid, - end_to_end_id = Eid}} - = Pkt, - #state{dpr = {X,H,E}} +recv1('DPA' = Name, + #diameter_header{hop_by_hop_id = Hid, end_to_end_id = Eid} + = H, + Msg, + #state{dpr = {X,HI,EI}} = S) - when H /= Hid; - E /= Eid; + when HI /= Hid; + EI /= Eid; not X -> - rcv(N, Pkt, S); + Pkt = pkt(H, Msg), + handle(Name, Pkt, S); -%% Any other message with a header and no length errors: send to the -%% parent. -recv1(Name, Pkt, #state{}) -> - {recv, Name, Pkt}. +%% Any other message with a header and no length errors. +recv1(Name, H, Msg, #state{parent = Pid, ack = Ack} = S) -> + Pkt = pkt(H, Msg), + Pid ! {recv, self(), get_route(Ack, Pkt), Name, Pkt}, + handle(Name, Pkt, S). -%% recv/3 +%% pkt/2 -recv(#diameter_header{length = Len} - = H, - #diameter_packet{bin = Bin} - = Pkt, - #state{length_errors = E} - = S) - when E == handle; - 0 == Len rem 4, bit_size(Bin) == 8*Len -> - recv(Pkt#diameter_packet{header = H}, S); +pkt(H, Bin) + when is_binary(Bin) -> + #diameter_packet{header = H, + bin = Bin}; -recv(#diameter_header{} - = H, - #diameter_packet{bin = Bin}, - #state{length_errors = E}) -> - T = {size(Bin), bit_size(Bin) rem 8, H}, - invalid(E, message_length_mismatch, T), - Bin; +pkt(H, Pkt) -> + Pkt#diameter_packet{header = H}. -recv(false, #diameter_packet{bin = Bin}, #state{length_errors = E}) -> - invalid(E, truncated_header, Bin), - Bin. +%% invalid/3 %% Note that counters here only count discarded messages. invalid(E, Reason, T) -> @@ -774,39 +769,39 @@ invalid(E, Reason, T) -> ?LOG(Reason, T), ok. -%% rcv/3 +%% handle/3 %% Incoming CEA. -rcv('CEA' = N, - #diameter_packet{header = #diameter_header{end_to_end_id = Eid, - hop_by_hop_id = Hid}} - = Pkt, - #state{state = {'Wait-CEA', Hid, Eid}} - = S) -> +handle('CEA' = N, + #diameter_packet{header = #diameter_header{end_to_end_id = Eid, + hop_by_hop_id = Hid}} + = Pkt, + #state{state = {'Wait-CEA', Hid, Eid}} + = S) -> ?LOG(recv, N), handle_CEA(Pkt, S); %% Incoming CER -rcv('CER' = N, Pkt, #state{state = recv_CER} = S) -> +handle('CER' = N, Pkt, #state{state = recv_CER} = S) -> handle_request(N, Pkt, S); %% Anything but CER/CEA in a non-Open state is an error, as is %% CER/CEA in anything but recv_CER/Wait-CEA. -rcv(Name, _, #state{state = PS}) +handle(Name, _, #state{state = PS}) when PS /= 'Open'; Name == 'CER'; Name == 'CEA' -> {stop, {Name, PS}}; -rcv('DPR' = N, Pkt, S) -> +handle('DPR' = N, Pkt, S) -> handle_request(N, Pkt, S); %% DPA in response to DPR, with the expected identifiers. -rcv('DPA' = N, - #diameter_packet{header = #diameter_header{end_to_end_id = Eid, - hop_by_hop_id = Hid} - = H} - = Pkt, +handle('DPA' = N, + #diameter_packet{header = #diameter_header{end_to_end_id = Eid, + hop_by_hop_id = Hid} + = H} + = Pkt, #state{dictionary = Dict0, transport = TPid, dpr = {X, Hid, Eid}, @@ -825,13 +820,13 @@ rcv('DPA' = N, %% Ignore an unsolicited DPA in particular. Note that dpa_timeout %% deals with the case in which the peer sends the wrong identifiers %% in DPA. -rcv('DPA' = N, #diameter_packet{header = H}, _) -> +handle('DPA' = N, #diameter_packet{header = H}, _) -> ?LOG(ignored, N), %% Note that these aren't counted in the normal recv counter. diameter_stats:incr({diameter_codec:msg_id(H), recv, ignored}), ok; -rcv(_, _, _) -> +handle(_, _, _) -> ok. %% incr/3 -- cgit v1.2.3 From 64e0dbd8f002d076f5d6c079f9166840c5fb17e3 Mon Sep 17 00:00:00 2001 From: Anders Svensson Date: Mon, 7 Aug 2017 02:59:49 +0200 Subject: Simplify extraction of incoming Diameter messages in diameter_tcp Appending to a binary is efficient, so just append message fragments Only match out the length once per message since doing so for every packet from TCP causes the binary to be copied. --- lib/diameter/src/transport/diameter_tcp.erl | 78 ++++++++++------------------- 1 file changed, 26 insertions(+), 52 deletions(-) diff --git a/lib/diameter/src/transport/diameter_tcp.erl b/lib/diameter/src/transport/diameter_tcp.erl index a2f393d5d4..e6168652de 100644 --- a/lib/diameter/src/transport/diameter_tcp.erl +++ b/lib/diameter/src/transport/diameter_tcp.erl @@ -87,8 +87,7 @@ module :: module() | undefined}). -type length() :: 0..16#FFFFFF. %% message length from Diameter header --type size() :: non_neg_integer(). %% accumulated binary size --type frag() :: {length(), size(), binary(), list(binary())} +-type frag() :: maybe_improper_list(length(), binary()) | binary(). -type connect_option() :: {raddr, inet:ip_address()} @@ -721,7 +720,7 @@ tls(accept, Sock, Opts) -> %% Receive packets until a full message is received, recv(Bin, #transport{frag = Head} = S) -> - case rcv(Head, Bin) of + case acc(Head, Bin) of {Msg, B} -> %% have a complete message ... message(recv, Msg, S#transport{frag = B}); Frag -> %% read more on the socket @@ -729,77 +728,52 @@ recv(Bin, #transport{frag = Head} = S) -> flush = false})) end. -%% rcv/2 +%% acc/2 -%% No previous fragment. -rcv(<<>>, Bin) -> - rcv(Bin); +%% Know how many bytes to extract. +acc([Len | Acc], Bin) -> + acc1(Len, <>); -%% Not even the first four bytes of the header. -rcv(Head, Bin) - when is_binary(Head) -> - rcv(<>); +%% Or not. +acc(Head, Bin) -> + acc(<>). -%% Or enough to know how many bytes to extract. -rcv({Len, N, Head, Acc}, Bin) -> - rcv(Len, N + size(Bin), Head, [Bin | Acc]). - -%% rcv/4 +%% acc1/3 %% Extract a message for which we have all bytes. -rcv(Len, N, Head, Acc) - when Len =< N -> - recv1(Len, bin(Head, Acc)); +acc1(Len, Bin) + when Len =< byte_size(Bin) -> + <> = Bin, + {Msg, Rest}; %% Wait for more packets. -rcv(Len, N, Head, Acc) -> - {Len, N, Head, Acc}. - -%% rcv/1 +acc1(Len, Bin) -> + [Len | Bin]. -%% Nothing left. -rcv(<<>> = Bin) -> - Bin; +%% acc/1 %% The Message Length isn't even sufficient for a header. Chances are %% things will go south from here but if we're lucky then the bytes we %% have extend to an intended message boundary and we can recover by %% simply receiving them. Make it so. -rcv(<<_:1/binary, Len:24, _/binary>> = Bin) +acc(<<_:1/binary, Len:24, _/binary>> = Bin) when Len < 20 -> {Bin, <<>>}; -%% Enough bytes to extract a message. -rcv(<<_:1/binary, Len:24, _/binary>> = Bin) - when Len =< size(Bin) -> - recv1(Len, Bin); - -%% Or not: wait for more packets. -rcv(<<_:1/binary, Len:24, _/binary>> = Head) -> - {Len, size(Head), Head, []}; +%% Know the message length. +acc(<<_:1/binary, Len:24, _/binary>> = Bin) -> + acc1(Len, Bin); %% Not even 4 bytes yet. -rcv(Head) -> - Head. - -%% recv1/2 - -recv1(Len, Bin) -> - <> = Bin, - {Msg, Rest}. - -%% bin/2 - -bin(Head, Acc) -> - list_to_binary([Head | lists:reverse(Acc)]). +acc(Bin) -> + Bin. %% bin/1 -bin({_, _, Head, Acc}) -> - bin(Head, Acc); +bin([_ | Bin]) -> + Bin; -bin(Bin) - when is_binary(Bin) -> +bin(Bin) -> Bin. %% flush/1 -- cgit v1.2.3 From 05e1764c1f7d079b5432bff11a792cdb31b00dcd Mon Sep 17 00:00:00 2001 From: Anders Svensson Date: Mon, 7 Aug 2017 15:35:11 +0200 Subject: Don't update diameter_tcp state unnecessarily Only reset the fragment after all messages have been extracted. --- lib/diameter/src/transport/diameter_tcp.erl | 22 +++++++++++----------- 1 file changed, 11 insertions(+), 11 deletions(-) diff --git a/lib/diameter/src/transport/diameter_tcp.erl b/lib/diameter/src/transport/diameter_tcp.erl index e6168652de..ead4e7e72a 100644 --- a/lib/diameter/src/transport/diameter_tcp.erl +++ b/lib/diameter/src/transport/diameter_tcp.erl @@ -598,11 +598,12 @@ t(T,S) -> %% Incoming packets. transition({P, Sock, Bin}, #transport{socket = Sock, - ssl = B} + ssl = B, + frag = Frag} = S) when P == ssl, true == B; P == tcp -> - recv(Bin, S#transport{active = false}); + recv(acc(Frag, Bin), S#transport{active = false}); %% Capabilties exchange has decided on whether or not to run over TLS. transition({diameter, {tls, Ref, Type, B}}, #transport{parent = Pid} @@ -719,14 +720,13 @@ tls(accept, Sock, Opts) -> %% using Nagle. %% Receive packets until a full message is received, -recv(Bin, #transport{frag = Head} = S) -> - case acc(Head, Bin) of - {Msg, B} -> %% have a complete message ... - message(recv, Msg, S#transport{frag = B}); - Frag -> %% read more on the socket - start_fragment_timer(setopts(S#transport{frag = Frag, - flush = false})) - end. + +recv({Msg, Rest}, S) -> %% have a complete message ... + recv(acc(Rest), message(recv, Msg, S)); + +recv(Frag, S) -> %% or not + start_fragment_timer(setopts(S#transport{frag = Frag, + flush = false})). %% acc/2 @@ -962,7 +962,7 @@ message(ack, _, #transport{message_cb = false} = S) -> S; message(Dir, Msg, #transport{message_cb = CB} = S) -> - recv(<<>>, actions(cb(CB, Dir, Msg), Dir, S)). + setopts(actions(cb(CB, Dir, Msg), Dir, S)). %% actions/3 -- cgit v1.2.3 From ca68fe8c449a170f64fd9b41b710ac67966be2ee Mon Sep 17 00:00:00 2001 From: Anders Svensson Date: Mon, 7 Aug 2017 15:41:50 +0200 Subject: Don't update diameter_tcp state unnecessarily Not with each setopts to re-activate the socket. --- lib/diameter/src/transport/diameter_tcp.erl | 27 +++++++++++++++++++-------- 1 file changed, 19 insertions(+), 8 deletions(-) diff --git a/lib/diameter/src/transport/diameter_tcp.erl b/lib/diameter/src/transport/diameter_tcp.erl index ead4e7e72a..bda92d02f6 100644 --- a/lib/diameter/src/transport/diameter_tcp.erl +++ b/lib/diameter/src/transport/diameter_tcp.erl @@ -603,7 +603,7 @@ transition({P, Sock, Bin}, #transport{socket = Sock, = S) when P == ssl, true == B; P == tcp -> - recv(acc(Frag, Bin), S#transport{active = false}); + recv(acc(Frag, Bin), S); %% Capabilties exchange has decided on whether or not to run over TLS. transition({diameter, {tls, Ref, Type, B}}, #transport{parent = Pid} @@ -724,9 +724,14 @@ tls(accept, Sock, Opts) -> recv({Msg, Rest}, S) -> %% have a complete message ... recv(acc(Rest), message(recv, Msg, S)); -recv(Frag, S) -> %% or not - start_fragment_timer(setopts(S#transport{frag = Frag, - flush = false})). +recv(Frag, #transport{recv = B, + socket = Sock, + module = M} + = S) -> %% or not + B andalso setopts(M, Sock), + start_fragment_timer(S#transport{frag = Frag, + flush = false, + active = B}). %% acc/2 @@ -885,14 +890,20 @@ setopts(#transport{socket = Sock, module = M} = S) when B, not A -> - case setopts(M, Sock, [{active, once}]) of - ok -> S#transport{active = true}; - X -> x({setopts, Sock, M, X}) %% possibly on peer disconnect - end; + setopts(M, Sock), + S#transport{active = true}; setopts(S) -> S. +%% setopts/2 + +setopts(M, Sock) -> + case setopts(M, Sock, [{active, once}]) of + ok -> ok; + X -> x({setopts, Sock, M, X}) %% possibly on peer disconnect + end. + %% portnr/2 portnr(gen_tcp, Sock) -> -- cgit v1.2.3 From d1f4369afb9fb9453ebe8868d88ae299d908a2e4 Mon Sep 17 00:00:00 2001 From: Anders Svensson Date: Tue, 8 Aug 2017 16:20:00 +0200 Subject: Avoid unnecessary copying of binaries in diameter_tcp Since messages are accumulated by appending binaries as of three commits back, the accumulated binary is prone to being copied, as discussed in the Efficiency Guide. Matching the Message Length header as bytes are being accumulated is one cause of this, so work around it by splitting the binary and extracting the length without a match. This doesn't feel like something that should be necessary: that matching a binary would cause an append to copy isn't obvious. The first attempt at simplifying the accumulation was simply to append an incoming binary to the current fragment, match against <<_, Len:24, _/binary>> to extract the length, and then test if there are enough bytes for a message. This lead to horrible performance (response times for 2 MB messages approximately 100 times worse than previously), and it wasn't at all obvious that the reason was the accumulated binary being copied with each append as a result of the match. Using split_binary avoids the match context that forces the copying. --- lib/diameter/src/transport/diameter_tcp.erl | 38 +++++++++++++++++++---------- 1 file changed, 25 insertions(+), 13 deletions(-) diff --git a/lib/diameter/src/transport/diameter_tcp.erl b/lib/diameter/src/transport/diameter_tcp.erl index bda92d02f6..6252dbddfb 100644 --- a/lib/diameter/src/transport/diameter_tcp.erl +++ b/lib/diameter/src/transport/diameter_tcp.erl @@ -748,8 +748,7 @@ acc(Head, Bin) -> %% Extract a message for which we have all bytes. acc1(Len, Bin) when Len =< byte_size(Bin) -> - <> = Bin, - {Msg, Rest}; + split_binary(Bin, Len); %% Wait for more packets. acc1(Len, Bin) -> @@ -757,17 +756,30 @@ acc1(Len, Bin) -> %% acc/1 -%% The Message Length isn't even sufficient for a header. Chances are -%% things will go south from here but if we're lucky then the bytes we -%% have extend to an intended message boundary and we can recover by -%% simply receiving them. Make it so. -acc(<<_:1/binary, Len:24, _/binary>> = Bin) - when Len < 20 -> - {Bin, <<>>}; - -%% Know the message length. -acc(<<_:1/binary, Len:24, _/binary>> = Bin) -> - acc1(Len, Bin); +%% Don't match on Bin since this results in it being copied at the +%% next append according to the Efficiency Guide. This is also the +%% reason that the Len is extracted and maintained when accumulating +%% messages. The simplest implementation is just to accumulate a +%% binary and match <<_, Len:24, _/binary>> each time the length is +%% required, but the performance of this decays quadratically with the +%% message length, since the binary is then copied with each append of +%% additional bytes from gen_tcp. + +acc(Bin) + when 3 < byte_size(Bin) -> + {Head, _} = split_binary(Bin, 4), + [_,A,B,C] = binary_to_list(Head), + Len = (A bsl 16) bor (B bsl 8) bor C, + if Len < 20 -> + %% Message length isn't sufficient for a Diameter Header. + %% Chances are things will go south from here but if we're + %% lucky then the bytes we have extend to an intended + %% message boundary and we can recover by simply receiving + %% them. Make it so. + {Bin, <<>>}; + true -> + acc1(Len, Bin) + end; %% Not even 4 bytes yet. acc(Bin) -> -- cgit v1.2.3 From 90b18515763cc332a1f2e612a24c50f245b5dc72 Mon Sep 17 00:00:00 2001 From: Anders Svensson Date: Tue, 8 Aug 2017 21:42:14 +0200 Subject: Split AVPs at decode Despite what the Efficiency Guide says about match being more efficient, split_binary appears to be slightly faster. (Although this one extraction is a drop in the bucket.) Binary optimizations aren't an issue during decode. --- lib/diameter/src/base/diameter_codec.erl | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/diameter/src/base/diameter_codec.erl b/lib/diameter/src/base/diameter_codec.erl index c171ea9dca..63e39b12d1 100644 --- a/lib/diameter/src/base/diameter_codec.erl +++ b/lib/diameter/src/base/diameter_codec.erl @@ -321,7 +321,7 @@ decode_avps('', _, _, _, #diameter_packet{header = H, %% unknown message %% msg = undefined identifies this case. decode_avps(MsgName, Mod, AppMod, Opts, #diameter_packet{bin = Bin} = Pkt) -> - <<_:20/binary, Avps/binary>> = Bin, + {_, Avps} = split_binary(Bin, 20), {Rec, As, Errors} = Mod:decode_avps(MsgName, Avps, Opts#{dictionary => AppMod, -- cgit v1.2.3 From 2be3e00698dea8236e60f3262c50b37eebfb4a04 Mon Sep 17 00:00:00 2001 From: Anders Svensson Date: Thu, 10 Aug 2017 11:13:51 +0200 Subject: Fix type spec That dialyzer hasn't noticed is broken. --- lib/diameter/src/base/diameter_traffic.erl | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/diameter/src/base/diameter_traffic.erl b/lib/diameter/src/base/diameter_traffic.erl index cfa857e09a..84ae33ae9c 100644 --- a/lib/diameter/src/base/diameter_traffic.erl +++ b/lib/diameter/src/base/diameter_traffic.erl @@ -186,7 +186,7 @@ incr_error(Dir, Id, TPid) -> %% --------------------------------------------------------------------------- -spec incr_rc(send|recv, Pkt, TPid, DictT) - -> {Counter, non_neg_integer()} + -> Counter | Reason when Pkt :: #diameter_packet{}, TPid :: pid(), -- cgit v1.2.3 From eb5fa7e2a0f67924828ea3d81891b86d08a53b0c Mon Sep 17 00:00:00 2001 From: Anders Svensson Date: Thu, 10 Aug 2017 11:14:14 +0200 Subject: Add service_opt() traffic_counters To be able to disable the counting of messages for which application callbacks take place. Messages sent/handled by diameter itself are always counted. --- lib/diameter/doc/src/diameter.xml | 20 ++ lib/diameter/src/base/diameter.erl | 1 + lib/diameter/src/base/diameter_config.erl | 3 + lib/diameter/src/base/diameter_service.erl | 1 + lib/diameter/src/base/diameter_traffic.erl | 295 ++++++++++++++++------------ lib/diameter/src/base/diameter_watchdog.erl | 2 +- 6 files changed, 191 insertions(+), 131 deletions(-) diff --git a/lib/diameter/doc/src/diameter.xml b/lib/diameter/doc/src/diameter.xml index 017f6bb01d..3ad24257a5 100644 --- a/lib/diameter/doc/src/diameter.xml +++ b/lib/diameter/doc/src/diameter.xml @@ -1053,6 +1053,26 @@ The default value is for backwards compatibility.

+ +{traffic_counters, boolean()} + +

+Whether or not to count application-specific messages; those for which +&man_app; callbacks take place. +If false then only messages handled by diameter itself are counted: +CER/CEA, DWR/DWA, DPR/DPA.

+ +

+Defaults to true.

+ + +

+Disabling counters is a performance improvement, but means that the +omitted counters are not returned by &service_info;.

+ + + + {use_shared_peers, boolean() | [node()] | evaluable()}

diff --git a/lib/diameter/src/base/diameter.erl b/lib/diameter/src/base/diameter.erl index c919ff4c93..2e18a1d903 100644 --- a/lib/diameter/src/base/diameter.erl +++ b/lib/diameter/src/base/diameter.erl @@ -353,6 +353,7 @@ call(SvcName, App, Message) -> | {sequence, sequence() | evaluable()} | {share_peers, remotes()} | {decode_format, decode_format()} + | {traffic_counters, boolean()} | {string_decode, boolean()} | {strict_arities, true | strict_arities()} | {strict_mbit, boolean()} diff --git a/lib/diameter/src/base/diameter_config.erl b/lib/diameter/src/base/diameter_config.erl index 2b069f40cc..f1b6e56782 100644 --- a/lib/diameter/src/base/diameter_config.erl +++ b/lib/diameter/src/base/diameter_config.erl @@ -715,6 +715,7 @@ make_config(SvcName, Opts) -> {true, strict_arities}, {true, strict_mbit}, {record, decode_format}, + {true, traffic_counters}, {true, string_decode}, {[], spawn_opt}]), @@ -760,6 +761,7 @@ opt(K, false = B) K == strict_arities; K == strict_mbit; K == decode_format; + K == traffic_counters; K == string_decode -> B; @@ -768,6 +770,7 @@ opt(K, true = B) K == use_shared_peers; K == strict_arities; K == strict_mbit; + K == traffic_counters; K == string_decode -> B; diff --git a/lib/diameter/src/base/diameter_service.erl b/lib/diameter/src/base/diameter_service.erl index c3c3c4b0e7..07f1fd3a4a 100644 --- a/lib/diameter/src/base/diameter_service.erl +++ b/lib/diameter/src/base/diameter_service.erl @@ -115,6 +115,7 @@ strict_arities => diameter:strict_arities(), strict_mbit := boolean(), decode_format := diameter:decode_format(), + traffic_counters := boolean(), string_decode := boolean(), spawn_opt := list() | {module(), atom(), list()}}}). diff --git a/lib/diameter/src/base/diameter_traffic.erl b/lib/diameter/src/base/diameter_traffic.erl index 84ae33ae9c..27a41d6eb0 100644 --- a/lib/diameter/src/base/diameter_traffic.erl +++ b/lib/diameter/src/base/diameter_traffic.erl @@ -70,12 +70,13 @@ timeout = 5000 :: 0..16#FFFFFFFF, %% for outgoing requests detach = false :: boolean()}). -%% Term passed back to receive_message/6 with every incoming message. +%% Term passed back to receive_message/5 with every incoming message. -record(recvdata, {peerT :: ets:tid(), service_name :: diameter:service_name(), apps :: [#diameter_app{}], sequence :: diameter:sequence(), + counters :: boolean(), codec :: #{decode_format := diameter:decode_format(), string_decode := boolean(), strict_arities => diameter:strict_arities(), @@ -98,12 +99,13 @@ %% --------------------------------------------------------------------------- make_recvdata([SvcName, PeerT, Apps, SvcOpts | _]) -> - #{sequence := {_,_} = Mask, spawn_opt := Opts} + #{sequence := {_,_} = Mask, spawn_opt := Opts, traffic_counters := B} = SvcOpts, {Opts, #recvdata{service_name = SvcName, peerT = PeerT, apps = Apps, sequence = Mask, + counters = B, codec = maps:with([decode_format, string_decode, strict_arities, @@ -197,18 +199,26 @@ incr_error(Dir, Id, TPid) -> | {'Experimental-Result', integer(), integer()}, Reason :: atom(). -incr_rc(Dir, Pkt, TPid, {_, AppDict, _} = DictT) -> - try - incr_result(Dir, Pkt, TPid, DictT) +incr_rc(Dir, Pkt, TPid, {MsgDict, AppDict, Dict0}) -> + incr_rc(Dir, Pkt, TPid, MsgDict, AppDict, Dict0); + +incr_rc(Dir, Pkt, TPid, Dict0) -> + incr_rc(Dir, Pkt, TPid, Dict0, Dict0, Dict0). + +%% incr_rc/6 + +incr_rc(Dir, Pkt, TPid, MsgDict, AppDict, Dict0) -> + try get_result(Dir, MsgDict, Dict0, Pkt) of + false -> + unknown; + Avp -> + incr_result(Dir, Avp, Pkt, TPid, AppDict) catch exit: {E,_} when E == no_result_code; E == invalid_error_bit -> incr(TPid, {msg_id(Pkt#diameter_packet.header, AppDict), Dir, E}), E - end; - -incr_rc(Dir, Pkt, TPid, Dict0) -> - incr_rc(Dir, Pkt, TPid, {Dict0, Dict0, Dict0}). + end. %% --------------------------------------------------------------------------- %% receive_message/5 @@ -307,14 +317,15 @@ recv_request(Ack, = Pkt, Dict0, #recvdata{peerT = PeerT, - apps = Apps} + apps = Apps, + counters = Count} = RecvData) -> Ack andalso (TPid ! {handler, self()}), case diameter_service:find_incoming_app(PeerT, TPid, Id, Apps) of {#diameter_app{id = Aid, dictionary = AppDict} = App, Caps} -> - incr(recv, Pkt, TPid, AppDict), + Count andalso incr(recv, Pkt, TPid, AppDict), DecPkt = decode(Aid, AppDict, RecvData, Pkt), - incr_error(recv, DecPkt, TPid, AppDict), + Count andalso incr_error(recv, DecPkt, TPid, AppDict), send_A(recv_R(App, TPid, Dict0, Caps, RecvData, DecPkt), TPid, App, @@ -535,6 +546,7 @@ send_A({call, Opts}, TPid, App, Dict0, RecvData, Pkt, Caps, Fs) -> MsgDict, AppDict, Dict0, + RecvData#recvdata.counters, Fs); RC -> send_answer(answer_message(RC, Dict0, Caps, Pkt), @@ -578,14 +590,22 @@ send_answer(Ans, TPid, MsgDict, AppDict, Dict0, RecvData, DecPkt, Fs) -> TPid, RecvData#recvdata.codec, make_answer_packet(Ans, DecPkt, MsgDict, Dict0)), - send_answer(Pkt, TPid, MsgDict, AppDict, Dict0, Fs). + send_answer(Pkt, + TPid, + MsgDict, + AppDict, + Dict0, + RecvData#recvdata.counters, + Fs). -%% send_answer/6 +%% send_answer/7 -send_answer(Pkt, TPid, MsgDict, AppDict, Dict0, [EvalPktFs | EvalFs]) -> +send_answer(Pkt, TPid, MsgDict, AppDict, Dict0, Count, [EvalPktFs | EvalFs]) -> eval_packet(Pkt, EvalPktFs), - incr(send, Pkt, TPid, AppDict), - incr_rc(send, Pkt, TPid, {MsgDict, AppDict, Dict0}), %% count outgoing + Count andalso begin + incr(send, Pkt, TPid, AppDict), + incr_rc(send, Pkt, TPid, MsgDict, AppDict, Dict0) + end, send(TPid, z(Pkt), _Route = self()), lists:foreach(fun diameter_lib:eval/1, EvalFs). @@ -1110,48 +1130,31 @@ find_avp(Code, VId, [_ | Avps]) -> %% Message sent as a header/avps list. incr_result(send = Dir, - #diameter_packet{msg = [#diameter_header{} = H | _]} - = Pkt, + Avp, + #diameter_packet{msg = [#diameter_header{} = H | _]}, TPid, - DictT) -> - incr_res(Dir, Pkt#diameter_packet{header = H}, TPid, DictT); - -%% Outgoing message as binary: don't count. (Sending binaries is only -%% partially supported.) -incr_result(send, #diameter_packet{header = undefined = No}, _, _) -> - No; + AppDict) -> + incr_result(Dir, Avp, H, [], TPid, AppDict); %% Incoming or outgoing. Outgoing with encode errors never gets here %% since encode fails. -incr_result(Dir, Pkt, TPid, DictT) -> - incr_res(Dir, Pkt, TPid, DictT). - -incr_res(Dir, - #diameter_packet{header = #diameter_header{is_error = E} - = Hdr, - errors = Es} - = Pkt, - TPid, - DictT) -> - {MsgDict, AppDict, Dict0} = DictT, +incr_result(Dir, Avp, Pkt, TPid, AppDict) -> + #diameter_packet{header = H, errors = Es} + = Pkt, + incr_result(Dir, Avp, H, Es, TPid, AppDict). +%% incr_result/6 + +incr_result(Dir, Avp, Hdr, Es, TPid, AppDict) -> Id = msg_id(Hdr, AppDict), %% Could be {relay, 0}, in which case the R-bit is redundant since %% only answers are being counted. Let it be however, so that the %% same tuple is in both send/recv and result code counters. %% Count incoming decode errors. - recv /= Dir orelse [] == Es orelse incr_error(Dir, Id, TPid, AppDict), - - %% Exit on a missing result code. - T = rc_counter(MsgDict, Dir, Pkt), - T == false andalso ?LOGX(no_result_code, {MsgDict, Dir, Hdr}), - {Ctr, RC, Avp} = T, - - %% Or on an inappropriate value. - is_result(RC, E, Dict0) - orelse ?LOGX(invalid_error_bit, {MsgDict, Dir, Hdr, Avp}), + send == Dir orelse [] == Es orelse incr_error(Dir, Id, TPid, AppDict), + Ctr = rcc(Avp), incr(TPid, {Id, Dir, Ctr}), Ctr. @@ -1196,7 +1199,50 @@ is_result(RC, true, _) -> incr(TPid, Counter) -> diameter_stats:incr(Counter, TPid, 1). -%% rc_counter/3 +%% rcc/1 + +rcc(#diameter_avp{name = 'Result-Code' = Name, value = V}) -> + {Name, head(V)}; + +rcc(#diameter_avp{name = 'Experimental-Result', value = V}) -> + head(V). + +%% head/1 + +head([V|_]) -> + V; +head(V) -> + V. + +%% rcv/1 + +rcv(#diameter_avp{name = N, value = V}) -> + rcv(N, head(V)). + +%% rcv/2 + +rcv('Experimental-Result', {_,_,N}) -> + N; + +rcv('Result-Code', N) -> + N. + +%% get_result/4 + +%% Message sent as binary: no checks or counting. +get_result(_, _, _, #diameter_packet{header = undefined}) -> + false; + +get_result(Dir, MsgDict, Dict0, Pkt) -> + Avp = get_result(MsgDict, msg(Dir, Pkt)), + Hdr = Pkt#diameter_packet.header, + %% Exit on a missing result code or inappropriate value. + Avp == false + andalso ?LOGX(no_result_code, {MsgDict, Dir, Hdr}), + E = Hdr#diameter_header.is_error, + is_result(rcv(Avp), E, Dict0) + orelse ?LOGX(invalid_error_bit, {MsgDict, Dir, Hdr, Avp}), + Avp. %% RFC 3588, 7.6: %% @@ -1204,46 +1250,29 @@ incr(TPid, Counter) -> %% applications MUST include either one Result-Code AVP or one %% Experimental-Result AVP. -rc_counter(Dict, Dir, #diameter_packet{header = H, - avps = As, - msg = Msg}) +%% msg/2 + +msg(Dir, #diameter_packet{header = H, + avps = As, + msg = Msg}) when Dir == recv; %% decoded incoming Msg == undefined -> %% relayed outgoing - rc_counter(Dict, [H|As]); - -rc_counter(Dict, _, #diameter_packet{msg = Msg}) -> - rc_counter(Dict, Msg). - -rc_counter(Dict, Msg) -> - rcc(get_result(Dict, Msg)). - -rcc(#diameter_avp{name = 'Result-Code' = Name, value = N} = A) - when is_integer(N) -> - {{Name, N}, N, A}; - -rcc(#diameter_avp{name = 'Result-Code' = Name, value = [N|_]} = A) - when is_integer(N) -> - {{Name, N}, N, A}; + [H|As]; -rcc(#diameter_avp{name = 'Experimental-Result', value = {_,_,N} = T} = A) - when is_integer(N) -> - {T, N, A}; - -rcc(#diameter_avp{name = 'Experimental-Result', value = [{_,_,N} = T|_]} = A) - when is_integer(N) -> - {T, N, A}; - -rcc(_) -> - false. +msg(_, #diameter_packet{msg = Msg}) -> + Msg. %% get_result/2 get_result(Dict, Msg) -> try [throw(A) || N <- ['Result-Code', 'Experimental-Result'], - #diameter_avp{} = A <- [get_avp(Dict, N, Msg)]] + #diameter_avp{} = A <- [get_avp(Dict, N, Msg)], + is_integer(catch rcv(A))], + false catch - #diameter_avp{} = A -> A + #diameter_avp{} = A -> + A end. x(T) -> @@ -1367,7 +1396,7 @@ make_opts([T | _], _, _, _, _, _) -> send_request({{TPid, _Caps} = TC, App} = Transport, - #{sequence := Mask} + #{sequence := Mask, traffic_counters := Count} = SvcOpts, Msg0, CallOpts, @@ -1383,9 +1412,15 @@ send_request({{TPid, _Caps} = TC, App} SvcOpts, ReqPkt), eval_packet(EncPkt, Fs), - T = send_R(ReqPkt, EncPkt, Transport, CallOpts, Caller, SvcName), + T = send_R(ReqPkt, + EncPkt, + Transport, + CallOpts, + Caller, + Count, + SvcName), Ans = recv_answer(SvcName, App, CallOpts, T), - handle_answer(SvcName, SvcOpts, App, Ans); + handle_answer(SvcName, Count, SvcOpts, App, Ans); {discard, Reason} -> {error, Reason}; discard -> @@ -1528,6 +1563,7 @@ send_R(ReqPkt, {{TPid, _Caps} = TC, #diameter_app{dictionary = AppDict}}, #options{timeout = Timeout}, {Pid, Ref}, + Count, SvcName) -> Req = #request{ref = Ref, caller = Pid, @@ -1535,7 +1571,7 @@ send_R(ReqPkt, peer = TC, packet = ReqPkt}, - incr(send, EncPkt, TPid, AppDict), + Count andalso incr(send, EncPkt, TPid, AppDict), {TRef, MRef} = zend_requezt(TPid, EncPkt, Req, SvcName, Timeout), Pid ! Ref, %% tell caller a send has been attempted {TRef, MRef, Req}. @@ -1567,15 +1603,16 @@ failover(SvcName, App, Req, CallOpts) -> CallOpts, SvcName). -%% handle_answer/4 +%% handle_answer/5 -handle_answer(SvcName, _, App, {error, Req, Reason}) -> +handle_answer(SvcName, _, _, App, {error, Req, Reason}) -> #request{packet = Pkt, peer = {_TPid, _Caps} = TC} = Req, cb(App, handle_error, [Reason, msg(Pkt), SvcName, TC]); handle_answer(SvcName, + Count, SvcOpts, #diameter_app{id = Id, dictionary = AppDict, @@ -1589,43 +1626,50 @@ handle_answer(SvcName, #request{peer = {TPid, _}} = Req, - incr(recv, DecPkt, TPid, AppDict), - - AnsPkt = try - incr_result(recv, DecPkt, TPid, {MsgDict, AppDict, Dict0}) - of - _ -> DecPkt - catch - exit: {no_result_code, _} -> - %% RFC 6733 requires one of Result-Code or - %% Experimental-Result, but the decode will have - %% detected a missing AVP. If both are optional in - %% the dictionary then this isn't a decode error: - %% just continue on. - DecPkt; - exit: {invalid_error_bit, {_, _, _, Avp}} -> - #diameter_packet{errors = Es} - = DecPkt, - E = {5004, Avp}, - DecPkt#diameter_packet{errors = [E|Es]} - end, - - handle_answer(AnsPkt, SvcName, App, AE, Req). + answer(answer(DecPkt, TPid, MsgDict, AppDict, Dict0, Count), + SvcName, + App, + AE, + Req). + +%% answer/6 + +answer(DecPkt, TPid, MsgDict, AppDict, Dict0, Count) -> + Count andalso incr(recv, DecPkt, TPid, AppDict), + try get_result(recv, MsgDict, Dict0, DecPkt) of + Avp -> + Count andalso false /= Avp + andalso incr_result(recv, Avp, DecPkt, TPid, AppDict), + DecPkt + catch + exit: {no_result_code, _} -> + %% RFC 6733 requires one of Result-Code or + %% Experimental-Result, but the decode will have + %% detected a missing AVP. If both are optional in + %% the dictionary then this isn't a decode error: + %% just continue on. + DecPkt; + exit: {invalid_error_bit, {_, _, _, Avp}} -> + #diameter_packet{errors = Es} + = DecPkt, + E = {5004, Avp}, + DecPkt#diameter_packet{errors = [E|Es]} + end. -%% handle_answer/5 +%% answer/5 -handle_answer(#diameter_packet{errors = Es} - = Pkt, - SvcName, - App, - AE, - #request{peer = {_TPid, _Caps} = TC, - packet = P}) +answer(#diameter_packet{errors = Es} + = Pkt, + SvcName, + App, + AE, + #request{peer = {_TPid, _Caps} = TC, + packet = P}) when callback == AE; [] == Es -> cb(App, handle_answer, [Pkt, msg(P), SvcName, TC]); -handle_answer(#diameter_packet{header = H}, SvcName, _, AE, _) -> +answer(#diameter_packet{header = H}, SvcName, _, AE, _) -> handle_error(H, SvcName, AE). %% handle_error/3 @@ -1838,10 +1882,8 @@ get_destination(Dict, Msg) -> [str(get_avp_value(Dict, D, Msg)) || D <- ['Destination-Realm', 'Destination-Host']]. -%% This is not entirely correct. The avp could have an arity 1, in -%% which case an empty list is a DiameterIdentity of length 0 rather -%% than the list of no values we treat it as by mapping to undefined. -%% This behaviour is documented. +%% A DiameterIdentity has length at least one, so an empty list is not +%% a Realm/Host. str([]) -> undefined; str(T) -> @@ -1871,10 +1913,8 @@ get_avp(?RELAY, Name, Msg) -> get_avp(Dict, Name, [#diameter_header{} | Avps]) -> try {Code, _, VId} = Dict:avp_header(Name), - find_avp(Code, VId, Avps) - of - A -> - (avp_decode(Dict, Name, ungroup(A)))#diameter_avp{name = Name} + A = find_avp(Code, VId, Avps), + (avp_decode(Dict, Name, ungroup(A)))#diameter_avp{name = Name} catch error: _ -> undefined @@ -1931,13 +1971,8 @@ avp_decode(Dict, Name, #diameter_avp{value = undefined, data = Bin} = Avp) when is_binary(Bin) -> - try Dict:avp(decode, Bin, Name, decode_opts(Dict)) of - V -> - Avp#diameter_avp{value = V} - catch - error:_ -> - Avp - end; + V = Dict:avp(decode, Bin, Name, decode_opts(Dict)), + Avp#diameter_avp{value = V}; avp_decode(_, _, #diameter_avp{} = Avp) -> Avp. diff --git a/lib/diameter/src/base/diameter_watchdog.erl b/lib/diameter/src/base/diameter_watchdog.erl index 294325751e..b2172356ee 100644 --- a/lib/diameter/src/base/diameter_watchdog.erl +++ b/lib/diameter/src/base/diameter_watchdog.erl @@ -154,7 +154,7 @@ i({Ack, T, Pid, {Opts, receive_data = RecvData, dictionary = Dict0, config = - maps:without(CodecKeys, + maps:without([traffic_counters | CodecKeys], config(SvcOpts#{restrict => restrict(Nodes), suspect => 1, okay => 3}, -- cgit v1.2.3 From fbc0fe7de4af5d24bd5ccd3eaf2417fbb436da27 Mon Sep 17 00:00:00 2001 From: Anders Svensson Date: Thu, 10 Aug 2017 12:01:38 +0200 Subject: Randomly disable traffic counters in traffic suite To exercise the new traffic_counters option. --- lib/diameter/test/diameter_traffic_SUITE.erl | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/lib/diameter/test/diameter_traffic_SUITE.erl b/lib/diameter/test/diameter_traffic_SUITE.erl index 1760d7c5dc..4e70a960c5 100644 --- a/lib/diameter/test/diameter_traffic_SUITE.erl +++ b/lib/diameter/test/diameter_traffic_SUITE.erl @@ -460,12 +460,17 @@ start_services(Config) -> server_decoding = SD} = Grp = group(Config), - ok = diameter:start_service(SN, [{decode_format, SD} + ok = diameter:start_service(SN, [{traffic_counters, bool()}, + {decode_format, SD} | ?SERVICE(SN, Grp)]), - ok = diameter:start_service(CN, [{sequence, ?CLIENT_MASK}, + ok = diameter:start_service(CN, [{traffic_counters, bool()}, + {sequence, ?CLIENT_MASK}, {strict_arities, decode} | ?SERVICE(CN, Grp)]). +bool() -> + 0.5 =< rand:uniform(). + add_transports(Config) -> #group{transport = T, encoding = E, -- cgit v1.2.3 From a73254692913e689aedf1154c0ee378546502501 Mon Sep 17 00:00:00 2001 From: Anders Svensson Date: Fri, 11 Aug 2017 01:46:30 +0200 Subject: Randomly skip groups in traffic suite To limit the number of testcases being run. --- lib/diameter/test/diameter_traffic_SUITE.erl | 14 +++++++++++--- 1 file changed, 11 insertions(+), 3 deletions(-) diff --git a/lib/diameter/test/diameter_traffic_SUITE.erl b/lib/diameter/test/diameter_traffic_SUITE.erl index 4e70a960c5..fde7e9b8c2 100644 --- a/lib/diameter/test/diameter_traffic_SUITE.erl +++ b/lib/diameter/test/diameter_traffic_SUITE.erl @@ -121,6 +121,9 @@ %% =========================================================================== +%% Fraction of shuffle/parallel groups to randomly skip. +-define(SKIP, 0.25). + %% Positive number of testcases from which to select (randomly) from %% tc(), the list of testcases to run, or [] to run all. The random %% selection is to limit the time it takes for the suite to run. @@ -316,9 +319,14 @@ end_per_suite(_Config) -> init_per_group(Name, Config) when Name == shuffle; Name == parallel -> - start_services(Config), - add_transports(Config), - [{sleep, Name == parallel} | Config]; + case rand:uniform() < ?SKIP of + true -> + {skip, random}; + false -> + start_services(Config), + add_transports(Config), + [{sleep, Name == parallel} | Config] + end; init_per_group(sctp = Name, Config) -> {_, Sctp} = lists:keyfind(Name, 1, Config), -- cgit v1.2.3 From d8d3c2a36045bcbe2ae5fd8f08b30a5d103f7bac Mon Sep 17 00:00:00 2001 From: Anders Svensson Date: Thu, 17 Aug 2017 00:53:36 +0200 Subject: Work around unexpected common_test behaviour diameter_traffic_SUITE has four layers of nested groups, so when a testcase is run it should get Config from four init_per_group plus one init_per_testcase. This isn't what happens: Config is being accumulated from several init_per_group in some manner that isn't clear, and the skip in the parent commit somehow results in growing test_server_gl:init/1 processes that eventually consume all memory. Replacing rather than prepending to Config in init_per_group works around this, but the common_test behaviour seems wrong. --- lib/diameter/test/diameter_traffic_SUITE.erl | 13 +++++++++++-- 1 file changed, 11 insertions(+), 2 deletions(-) diff --git a/lib/diameter/test/diameter_traffic_SUITE.erl b/lib/diameter/test/diameter_traffic_SUITE.erl index fde7e9b8c2..a2c0f7fae6 100644 --- a/lib/diameter/test/diameter_traffic_SUITE.erl +++ b/lib/diameter/test/diameter_traffic_SUITE.erl @@ -325,7 +325,7 @@ init_per_group(Name, Config) false -> start_services(Config), add_transports(Config), - [{sleep, Name == parallel} | Config] + replace({sleep, Name == parallel}, Config) end; init_per_group(sctp = Name, Config) -> @@ -352,7 +352,7 @@ init_per_group(Name, Config) -> server_decoding = D, server_sender = SS, server_throttle = ST}, - [{group, G}, {runlist, select()} | Config]; + replace([{group, G}, {runlist, select()}], Config); _ -> Config end. @@ -396,6 +396,15 @@ init_per_testcase(Name, Config) -> end_per_testcase(_, _) -> ok. +%% replace/2 + +replace(Pairs, Config) + when is_list(Pairs) -> + lists:foldl(fun replace/2, Config, Pairs); + +replace({Key, _} = T, Config) -> + [T | lists:keydelete(Key, 1, Config)]. + %% -------------------- %% Testcases to run when services are started and connections -- cgit v1.2.3