-
Notifications
You must be signed in to change notification settings - Fork 3k
add {active,N} socket option for TCP, UDP, and SCTP #66
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Just a quick reflex reaction. Does excluding -32768 add any significant merit? If not I think it will only surprise someone some day since not all / Raimo On Wed, Sep 04, 2013 at 07:41:57AM -0700, Steve Vinoski wrote:
/ Raimo Niskanen, Erlang/OTP, Ericsson AB |
@RaimoNiskanen thanks for the feedback. Allowing that value would be fine. I excluded it only because I felt it was slightly odd that you could subtract more than you could add, but I also know that's not a very strong argument given that the socket's count can never be set to a negative number regardless. I'm happy to amend the commit to take your feedback into account, just let me know. |
I think this is a great feature! The only thing I want to note is that this should probably be in the R17-track.It's a pretty significant feature addition to be thrown in as a patch. In any regard it won't be in R16B02. |
@psyeugenic I assumed it would be considered only for R17 as well, but when I asked if it should be off maint or master I was told maint because it allowed the OTP team the flexibility to take it in either direction. |
Ah, so we have been over this, goody. I'm still acquiring conversations I missed during my vacation. It will be R17 then. I'll guess someone at OTP will rebase it when suitable. Topic branches in maint (opu) are currently being drained due to stabilization before release. |
Patch has passed first testings and has been assigned to be reviewed |
I've amended the commit on this branch to address the -32768 issue that @RaimoNiskanen raised. |
Unable to apply diff |
Add the {active,N} socket option, where N is an integer in the range -32768..32767, to allow a caller to specify the number of data messages to be delivered to the controlling process. Once the socket's delivered message count either reaches 0 or is explicitly set to 0 with inet:setopts/2 or by including {active,0} as an option when the socket is created, the socket transitions to passive ({active, false}) mode and the socket's controlling process receives a message to inform it of the transition. TCP sockets receive {tcp_passive,Socket}, UDP sockets receive {udp_passive,Socket} and SCTP sockets receive {sctp_passive,Socket}. The socket's delivered message counter defaults to 0, but it can be set using {active,N} via any gen_tcp, gen_udp, or gen_sctp function that takes socket options as arguments, or via inet:setopts/2. New N values are added to the socket's current counter value, and negative numbers can be used to reduce the counter value. Specifying a number that would cause the socket's counter value to go above 32767 causes an einval error. If a negative number is specified such that the counter value would become negative, the socket's counter value is set to 0 and the socket transitions to passive mode. If the counter value is already 0 and inet:setopts(Socket, [{active,0}]) is specified, the counter value remains at 0 but the appropriate passive mode transition message is generated for the socket. This commit contains a modified preloaded prim_inet.beam due to changes in prim_inet.erl. Add tests for {active,N} mode for TCP, UDP, and SCTP sockets. Add documentation for {active,N} mode for inet, gen_tcp, gen_udp, and gen_sctp.
The "unable to apply diff" error was due to the netns change on the maint branch. I've rebased maint out to my branch to resolve this. |
Patch has passed first testings and has been assigned to be reviewed |
I would like to understand the exact difference between
Intuitively, those would be point-wise equivalent. Please correct me if I am wrong, and if so ... I think the documentation should include a description of such semantic differences. I'm asking because it seems that it would be nice to have this defined as a proper generalisation of the existing semantics, so that we can eliminate the code that handles active/passive cases. The inet driver is complicated enough as it is :-) |
The differences lie in the fact that when For example, consider a TCP socket named
Setting |
Will setting the socket to I figure it won't (because it wouldn't make much sense given the connection is still active), but I can't help but feel there is an inconsistency in how Isn't the ambiguity of the
Why should I guess the argument should come down to a preference on whether we want to track the I think there's good points to be had for both, but we should be careful when making that decision? |
If a socket is in The
Of these choices, the last one, which is what this patch implements, is the cleanest.
And just as The points 1-5 you describe are race conditions resulting from the fact that messages can arrive on a socket concurrently with the application making calls to change active/passive modes on that socket. As your descriptions already note, these race conditions are already possible in Erlang today, and this patch neither improves nor worsens the situation, as that is not its focus. In fact, I would argue that while the explicit transitions of the socket's active mode between In practice, the active/passive modes are about flow control and are used to engage backpressure mechanisms in protocols that provide them. Today, for active modes we have only |
Thanks Steve. I see it now. But I'll still argue that it's fairly complicated, and likely to cause confusion. Perhaps we should call it something other than |
Here's another thought: as a client if a stream socket, most likely I'm interested in receiving a limited number of bytes, not necessarily a limited number of MTU units of data; as would probably typically be the consequence of using |
I don't think a new name is needed since this new option fits perfectly with the other existing As for treating
|
Merged. |
Great! |
John/beamasm/fix div rem shrink
Fix incorect operator in GenerateUcd.py (modulo -> bitwise and) Co-authored-by: Zoltan Herczeg <[email protected]>
fix: merge nodes for external-copy cstruct
Add the {active,N} socket option, where N is an integer in the range
-32767..32767, to allow a caller to specify the number of data messages to
be delivered to the controlling process. Once the socket's delivered
message count either reaches 0 or is explicitly set to 0 with
inet:setopts/2 or by including {active,0} as an option when the socket is
created, the socket transitions to passive ({active, false}) mode and the
socket's controlling process receives a message to inform it of the
transition. TCP sockets receive {tcp_passive,Socket}, UDP sockets receive
{udp_passive,Socket} and SCTP sockets receive {sctp_passive,Socket}.
The socket's delivered message counter defaults to 0, but it can be set
using {active,N} via any gen_tcp, gen_udp, or gen_sctp function that takes
socket options as arguments, or via inet:setopts/2. New N values are added
to the socket's current counter value, and negative numbers can be used to
reduce the counter value. Specifying a number that would cause the socket's
counter value to go above 32767 causes an einval error. If a negative
number is specified such that the counter value would become negative, the
socket's counter value is set to 0 and the socket transitions to passive
mode. If the counter value is already 0 and inet:setopts(Socket,
[{active,0}]) is specified, the counter value remains at 0 but the
appropriate passive mode transition message is generated for the socket.
Note that the range for N is intentionally -32767..32767, not the entire
signed 16-bit integer range -32768..32767. This is because the smallest
negative value, -32767, need only have the same absolute value as the
32767, the maximum allowed N value.
This commit contains a modified preloaded prim_inet.beam due to changes in
prim_inet.erl.
Add tests for {active,N} mode for TCP, UDP, and SCTP sockets.
Add documentation for {active,N} mode for inet, gen_tcp, gen_udp, and
gen_sctp.