[erlang-patches] Non-blocking SCTP connect [Was: [erlang-patches] SCTP improvements]

Raimo Niskanen raimo+erlang-patches@REDACTED
Mon Jan 25 16:12:05 CET 2010


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


More information about the erlang-patches mailing list