On Mon, Dec 21, 2009 at 10:57:22PM -0800, Simon Cornish wrote:
> Just in time for Christmas, a non-blocking SCTP connect.
>
> git fetch git://github.com/dotsimon/otp.git sctp_connect_nowait
>
> I decided to name the functions connect_init to better reflect the
> behaviour since there can actually be some waiting if the peer address
> needs resolving.
>
> The commit comment hopefully contains enough information to update the
> documentation. I did not want to mess with the XML directly since
> there is no way to build and see it looks ok.
>
> Regards,
> Simon
commit: ffbf3fe3ce632567cacacdff73a3cc87ab6d9c6d
Sorry about the long delay again. This diff looks functionally fine
and the name choice is fine. Just a matter of coding style:
All functions in the module gen_sctp are careful to type
check in the API function or erlang:error(badarg, ArgList)
if the type checks fail; from the API function.
(Perhaps not the same arity but anyway...)
So the helper function do_connect/6 calling erlang:error(badarg, _)
with an incomplete argument list should be improved. This would
be confusing since the API user did not call do_connect/6.
The alternative way would be to let do_connect/6 just bail out
in a function clause, but that is not the style of this module.
So, maybe something like this in gen_sctp.erl:
connect(S, Addr, Port, Opts, Timeout) ->
case do_connect(S, Addr, Port, Opts, Timeout, true) of
Reason when is_atom(Reason) ->
erlang:error(Reason, [S, Addr, Port, Opts, Timeout);
Result ->
Result
end.
:
connect_init(S, Addr, Port, Opts, Timeout) ->
case do_connect(S, Addr, Port, Opts, Timeout, false) of
Reason when is_atom(Reason) ->
erlang:error(Reason, [S, Addr, Port, Opts, Timeout);
Result ->
Result
end.
:
do_connect(S, Addr, Port, Opts, Timeout, ConnWait) when is_port(S), is_list(Opts) ->
case inet_db:lookup_socket(S) of
{ok,Mod} ->
case Mod:getserv(Port) of
{ok,Port} ->
try inet:start_timer(Timeout) of
Timer ->
try Mod:getaddr(Addr, Timer) of
{ok,IP} ->
ConnectTimer =
if ConnWait ->
Timer;
true ->
nowait
end,
Mod:connect(S, IP, Port, Opts, ConnectTimer);
Error -> Error
after
inet:stop_timer(Timer)
end
catch
error:Reason ->
Reason
end;
Error -> Error
end;
Error -> Error
end;
do_connect(S, Addr, Port, Opts, Timeout) ->
badarg.
>
> ps. I see that unfortunately somewhere between GitX, git and github
> the line-wrapping in my commit comment has been horribly mangled. I
> think it's just readable!
>
> On Wed, Dec 16, 2009 at 10:27 AM, Simon Cornish
> zl9d97p02-at-sneakemail.com |erlang-mail/gmail|
> <6g9sko63ub0t@REDACTED> wrote:
> > Hi Raimo,
> >
> > I'm taking this as a separate thread for each topic.
> >
> > On Wed, Dec 16, 2009 at 4:42 AM, Raimo Niskanen
> > raimo+erlang-patches-at-erix.ericsson.se wrote:
> > [...]
> >> http://github.com/erlang/otp/commit/92f3ceea0fa51df308f6f2689ff82857b6442c30
> >> Implement a non-blocking gen_sctp:connect
> >
> > I think there are plenty of examples in OTP where library functions
> > return a different type based on the argument(s). But, subjective
> > comments on style aside (since I have no style :-) I had not
> > considered that this was changing existing interface semantics in a
> > service release.
> >
> > I'm happy to rewrite it as new connect_nowait functions as you
> > suggest. What's the best way to do this? Amended commit? New commit?
> > New branch? Maybe Björn has an opinion since he has to do the messy
> > part...
> >
> > Regards,
> > Simon
> >
> >>
> >> lib/kernel/src/inet_sctp.erl: 74/82
> >> Comparision Timeout /= 0 should have been Timeout =/= 0
> >> just to be certain...
> >>
> >> To compare Timeout =/= 0 is unfortunately also wrong since
> >> time may have elapsed since inet:start_timer/1 was
> >> called and inet:timeout/1 may return 0 when the original
> >> Timeout actually was nonzero.
> >>
> >> And, furthermore; having gen_sctp:connect/4,5 to return a
> >> different type for some value (i.e 0) of an argument
> >> (i.e Timeout) is considered ugly. And introducing such
> >> a change in a service release is out of the question.
> >>
> >> Therefore; can you rewrite your patch to introduce a new
> >> function gen_sctp:connect_nowait/4 that does exactly
> >> what your gen_sctp:connect(S, Add, Port, Opts, 0) did?
> >>
> >> There will be some code duplication, but you can maybe
> >> also avoid creating a timer alltogether and perhaps
> >> use a special value (e.g 0) to inet_sctp:connect/5,
> >> last argument, to not have to duplicate everything.
> >> inet_sctp is a non-public interface so it can be altered.
> >> Doing it this way also avoids fixing inet6_sctp.erl.
> >>
> >
> > ________________________________________________________________
> > erlang-patches mailing list. See http://www.erlang.org/faq.html
> > erlang-patches (at) erlang.org
> >
> >
>
> ________________________________________________________________
> erlang-patches mailing list. See http://www.erlang.org/faq.html
> erlang-patches (at) erlang.org
>
--
/ Raimo Niskanen, Erlang/OTP, Ericsson AB