Inherit options in gen_tcp:accept/1 in inet_drv directly #283

wants to merge 2 commits into from

8 participants

nox commented Mar 9, 2014

I can't test whether 'priority' and 'tos' are properly inherited here as OS X doesn't support these options.

Cc @reedr.


Patch has passed first testings and has been assigned to be reviewed



Just put this though our testing and it seems like this patch breaks a number of testcases in kernel (and other applications) related to distribution: global_SUITE, erl_prim_loader_SUITE and many more.

nox commented May 7, 2014

Thanks. Will check these.


Found the bug, the inherited options should be put in tcp_inet_ctl(), not in tcp_inet_copy() directly.

@nox nox referenced this pull request May 17, 2014
@reedr reedr Optimize prim_inet:accept to eliminate setopts settings and inherit v…
…ia parent socket and inet_drv. Optimize inet:tcp_controlling_process to eliminate inet:setops calls where possible.

Is that one out of the CI system?

Erlang/OTP member

@garazdawi is assigned to review this and he's currently on vacation. There is only a skeleton crew here.


Holidays > whatever patch. =)

reedr and others added some commits Mar 16, 2012
@reedr reedr Inherit options in gen_tcp:accept/1 in inet_drv directly
This removes a getopts/setopts roundtrip in prim_inet:accept/1.
@nox nox Test that options are correctly inherited in gen_tcp:accept/1
The following options should be inherited: active, nodelay, keepalive,
delay_send, priority and tos.

I don't understand how the inet loader works enough to fix the failing tests. It seems that the direct inheriting of the active option breaks starting slave nodes in test_server. Could someone enlighten me on this?


The summary line of the commit message is too long and/or ends with a "."
Make sure the whole message follows the guidelines here:

Bad message: Test that options are correctly inherited in gen_tcp:accept/1

I am a script, I am not human

nox commented Nov 1, 2014

Was @OTP-Maintainer's length limit changed recently, or did it previously fail to look at all commits of a pull request?


The length limit was changed, and It previously failed to look at all commits. And some other minor changes.

nox commented Nov 3, 2014

@proxyles What's the limit now? 50?


@nox the 100 latest open pull requests. So we "should" not be able to miss anything.

nox commented Nov 3, 2014

I was talking about the length limit. :)


ah, no that's still set at 60 to be nice (50 is the recommended length in the guidelines)


Under the current circumstances e.i. the patch breaking existing functionality,
the test case needs some work. The test sets active true, which is default and hence not a good test.
And no figures backing up that the optimization is worth while. And looking at our current backlog this has a hard time making it to the top under these circumstance.


Not like I asked how it could be fixed and improved… As for whether it's worth it or not, personally I find it not straightforward at all that any accept call would cause an unnecessary roundtrip for a handful of optional settings.


I am interested in this patch now that I understand its purpose. Any chance you can give the man his feedback? :-)


If that question had been easy to answer, we would happily have done so. But nobody knows without spending a lot of time to investigating it. I am not saying that patch is not interesting, just that it currently is not interesting enough for us to spend a lot of time on. So for it to happen somebody else
will need to investigate and make all test pass. Also as I said the test case needs improving by setting a non default value.

@IngelaAndin IngelaAndin reopened this Apr 15, 2015

Closed until someone get back to this

@proxyles proxyles closed this Apr 6, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment