[erlang-patches] Non-blocking SCTP connect [Was: [erlang-patches] SCTP improvements]
Raimo Niskanen
raimo+erlang-patches@REDACTED
Tue Jan 26 10:14:34 CET 2010
On Mon, Jan 25, 2010 at 07:52:39AM -0800, Simon Cornish wrote:
> Hi,
> Is this something you would like me to refine my commit for and push
> back at Björn? I ask because the whole workflow here is still not
Yes please refine, I was unclear.
> clear to me. Nor, really, is the level of volunteer work to fix
> something that is clearly broken in the current releases.
We are grateful of any level of volunteer work we can get.
Without voluenteer work this fix of something that is
clearly broken would have to be weighed against other
equally or more important matters. We have no paying
customers that need this from SCTP...
> A question about your suggestion inline with the code below:
> Regards,
> Simon
>
> On Mon, Jan 25, 2010 at 7:12 AM, Raimo Niskanen
> raimo+erlang-patches-at-erix.ericsson.se |erlang-mail/gmail|
> <owq599gb3t@REDACTED> wrote:
> >
> > 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.
> >
>
> Shouldn't the Reason here be just badarg? do_connect may return ok
> which is also an atom, and generically speaking the badarg from the
> function clause is the only case for erlang:error
You are quite right. My mistake to be over-general.
This is partly why I think it is better you finish this.
It is more likely I make a silly mistake than you since you
already are into the code.
>
> /Simon
>
> > :
> >
> > 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
--
/ Raimo Niskanen, Erlang/OTP, Ericsson AB
More information about the erlang-patches
mailing list