ninenines / ranch

Socket acceptor pool for TCP protocols.
ISC License
1.19k stars 336 forks source link

Minor: ranch_ssl:listen can return non-atom error reasons #346

Open gomoripeti opened 5 months ago

gomoripeti commented 5 months ago

According to the ranch_transport behaviour, listen/1 callback should return only atoms as error reasons https://github.com/ninenines/ranch/blob/master/src/ranch_transport.erl#L31. However ssl:listen and hence ranch_ssl:listen/1 can return more sophisticated error reasons as visible in ssl:format_error/1.

A quick fix would be to use ssl:format_error instead of inet:format_error in (https://github.com/ninenines/ranch/blob/a8f31f3f0274f7e5a6b58fa6b6090c3160c4d023/src/ranch_acceptors_sup.erl#L114) (the former fall back to the later so works for non-ssl connections too)

Maybe a more generic solution would be if Transport behaviour would have an optional format_error callback and ranch_ssl would implement it calling ssl:format_error.

Not sure which one is preferred?

As an example the below crash happens if a non-existing cipher is provided in the options (seen with Erlang 26.2.5)

{failed_to_start_child,ranch_acceptors_sup,
 {badarg,
  [{erlang,atom_to_binary,
    [{options,
      {ciphers,
       [
       ...
       ]}}],
    [{error_info,#{module => erl_erts_errors}}]},
   {erl_posix_msg,message_1,1,
    [{file,"erl_posix_msg.erl"},{line,175}]},
   {erl_posix_msg,message,1,
    [{file,"erl_posix_msg.erl"},{line,29}]},
   {ranch_acceptors_sup,listen_error,5,
    [{file,"src/ranch_acceptors_sup.erl"},
     {line,95}]},
   {ranch_acceptors_sup,start_listen_sockets,5,
    [{file,"src/ranch_acceptors_sup.erl"},
     {line,54}]},
   {ranch_acceptors_sup,init,1,
    [{file,"src/ranch_acceptors_sup.erl"},
     {line,34}]},
essen commented 5 months ago

An optional format_error callback is probably the best solution.

gomoripeti commented 5 months ago

thanks for the feedback, I will work on a patch