Created attachment 183 [details]
no send callback invoked

you need to set a finit tx buffer size to see the bug.

For example:
build/debug/examples/tcp-2way-test --ns3::TcpSocket::SndBufSize=10012

the last byte of our 10000 bytes packet is never transmitted because HandleSend is never invoked by the TCP code. (it is called but only once and from the TalkerApp code ::SendPacket so, that does not count)

gdb shows that we don't invoke NotifySend because m_wouldBlock is not true. I have to ask: why don't we invoke NotifySend uncondtionally when we receive an ACK ? i.e., what is the purpose of m_wouldBlock ?

Created attachment 185 [details]
fix bug

trying to push patch into RC3.

Created attachment 187 [details]
another fix

(In reply to comment #3)
> gdb shows that we don't invoke NotifySend because m_wouldBlock is not true. I
> have to ask: why don't we invoke NotifySend uncondtionally when we receive an
> ACK ? i.e., what is the purpose of m_wouldBlock ?
> 

http://code.nsnam.org/ns-3-dev/rev/d65486147a02

The gist of Tom's rationale as I recall was that NotifySend is supposed to let an application which would be blocked on a send know that tx buffer space has cleared up.  If the application hasn't "blocked" the socket yet, there is no need to fire up the notification at all (return value of send can be used to glean this information).

But what's happening in this test case is that you are writing exactly as much into the buffer as is available, not "blocking" the socket according to our logic, which is flawed.  I've attached a fix that set m_wouldBlock to true in the case of sending exactly as much as is available in the buffer.

Can someone verify that this fixes the issue?  I didn't look too closely into the expected behavior of this iteration of the tcp-2way code, but my fix causes one more byte to be transferred than the old code did (under Mathieu's test case).

Hi.  About this issue, like I kept saying numerous times, IMHO NS-3 sockets should imitate an asynchronous socket framework using poll/select, or something like GLib which is based on a main loop that keeps calling poll().  For the simple reason that these APIs are well known are tested to work in real life.

When you call poll() with socket fd the poll() system call returns whenever there is output buffer space available in a socket, not when writing was blocked before and just unblocked.  The epoll(7) linux man page has some interesting information regarding level-triggered vs edge-triggered events.

I think Mathieu's fix is simpler both for implementation and for the application writer.  No need for apps to keep track of the Send return value to see if all bytes were written or not.  It can simply wait for the next send callback in order to send more bytes.  It's a simpler logic, really.  And it is close enough to the poll() model.

(In reply to comment #7)
> I think Mathieu's fix is simpler both for implementation and for the
> application writer.  No need for apps to keep track of the Send return value to
> see if all bytes were written or not.  It can simply wait for the next send
> callback in order to send more bytes.

Hm.. on second thought it is not as simple as that :-/  If the buffer is very lightly filled, waiting for the next ack is not going to help; better just to send as much data as possible until Send() returns less than packet->GetSize().

Ok.  _Ideally_, in a poll based main loop, while there is TX buffer space available in a socket, the output callback is called once for each main loop iteration, until the condition is no longer satisfied.  Thus, in these frameworks what absolutely never happens is for the send callback to be called recursively multiple times.  But once a send callback returns, it is scheduled to be called again if there is still more buffer space available.

I'm not 100% sure, but I think I know how to implement this in NS-3, even without using a per-Node Poll() object managing sockets.  Instead I would use Simulator::ScheduleNow and some socket state variable.

(In reply to comment #6)
> Created an attachment (id=187) [edit]
> another fix
> 
> (In reply to comment #3)
> > gdb shows that we don't invoke NotifySend because m_wouldBlock is not true. I
> > have to ask: why don't we invoke NotifySend uncondtionally when we receive an
> > ACK ? i.e., what is the purpose of m_wouldBlock ?
> > 
> 
> http://code.nsnam.org/ns-3-dev/rev/d65486147a02
> 
> The gist of Tom's rationale as I recall was that NotifySend is supposed to let
> an application which would be blocked on a send know that tx buffer space has
> cleared up.  If the application hasn't "blocked" the socket yet, there is no
> need to fire up the notification at all (return value of send can be used to
> glean this information).

There is no such thing as an application being blocked. The semantics of that callback are that it should be invoked when there is new room in the tx buffer so, the whole logic of m_wouldBlock is flawed. Don't try to invent smart logic where there should be none, please.

(In reply to comment #9)
> (In reply to comment #6)
> > Created an attachment (id=187) [edit]
> > another fix
> > 
> > (In reply to comment #3)
> > > gdb shows that we don't invoke NotifySend because m_wouldBlock is not true. I
> > > have to ask: why don't we invoke NotifySend uncondtionally when we receive an
> > > ACK ? i.e., what is the purpose of m_wouldBlock ?
> > > 
> > 
> > http://code.nsnam.org/ns-3-dev/rev/d65486147a02
> > 
> > The gist of Tom's rationale as I recall was that NotifySend is supposed to let
> > an application which would be blocked on a send know that tx buffer space has
> > cleared up.  If the application hasn't "blocked" the socket yet, there is no
> > need to fire up the notification at all (return value of send can be used to
> > glean this information).
> 
> There is no such thing as an application being blocked. The semantics of that
> callback are that it should be invoked when there is new room in the tx buffer
> so, the whole logic of m_wouldBlock is flawed. Don't try to invent smart logic
> where there should be none, please.
> 

The doxygen is misaligned with the tcp implementation-- that is my fault for not updating the doxygen.

I don't know what you mean "there is no such thing as an application being blocked".  We could have the callback invoked every time new space is available, sure.  However, this just pushes the m_wouldBlock flag into the application (in the sense that the application needs to ignore most of these callbacks but only act on them when its send() has recently failed).  It seemed to me that the way the TCP implementation is right now is closer to reality.

(In reply to comment #7)
> Hi.  About this issue, like I kept saying numerous times, IMHO NS-3 sockets
> should imitate an asynchronous socket framework using poll/select, or something
> like GLib which is based on a main loop that keeps calling poll().  For the
> simple reason that these APIs are well known are tested to work in real life.
> 
> When you call poll() with socket fd the poll() system call returns whenever
> there is output buffer space available in a socket, not when writing was
> blocked before and just unblocked.  The epoll(7) linux man page has some
> interesting information regarding level-triggered vs edge-triggered events.

Yes, we have a function already for that behavior:  GetTxAvailable().

I thought that the condition we were trying to detect with this callback was the transition event when the return value of the poll changed due to space becoming available.
 
> 
> I think Mathieu's fix is simpler both for implementation and for the
> application writer.  No need for apps to keep track of the Send return value to
> see if all bytes were written or not.  It can simply wait for the next send
> callback in order to send more bytes.  It's a simpler logic, really.  And it is
> close enough to the poll() model.

I'm not sure it is so simple to ignore the send return value.  How do you know when your send has failed?  Basically, the logic is now that you can keep sending as long as the return value is OK; when it is not, you are now in the conceptual blocked state and your send callback is later called when this state resolves itself.

> > Hi.  About this issue, like I kept saying numerous times, IMHO NS-3 sockets
> > should imitate an asynchronous socket framework using poll/select, or something
> > like GLib which is based on a main loop that keeps calling poll().  For the
> > simple reason that these APIs are well known are tested to work in real life.
> > 
> > When you call poll() with socket fd the poll() system call returns whenever
> > there is output buffer space available in a socket, not when writing was
> > blocked before and just unblocked.  The epoll(7) linux man page has some
> > interesting information regarding level-triggered vs edge-triggered events.
> 
> Yes, we have a function already for that behavior:  GetTxAvailable().
> 
> I thought that the condition we were trying to detect with this callback was
> the transition event when the return value of the poll changed due to space
> becoming available.
> 
> > 
> > I think Mathieu's fix is simpler both for implementation and for the
> > application writer.  No need for apps to keep track of the Send return value to
> > see if all bytes were written or not.  It can simply wait for the next send
> > callback in order to send more bytes.  It's a simpler logic, really.  And it is
> > close enough to the poll() model.
> 
> I'm not sure it is so simple to ignore the send return value.  How do you know
> when your send has failed?  Basically, the logic is now that you can keep
> sending as long as the return value is OK; when it is not, you are now in the
> conceptual blocked state and your send callback is later called when this state
> resolves itself.

If you do this, the application has to write a new code path when the Send call fails so, it has to handle the complicated/fragmentation case anyway. So, it is vastly simpler at the application layer to handle the complicated case all the time because that saves you a couple of extra if statements and makes sure that the exceptional code path is actually tested.

But, really, the issue here is that the API we defined and I agreed to for this callback is to be invoked whenever there is new space in the tx buffer and with good reason: all the smart heuristic you will devise in the TCP layer to save yourself a couple of function calls is just that: a heuristic and it is bound to fail in strange and unexpected ways down the road. 

I have spent a considerable amount of time coming up with this API when I first proposed it so, I would be very wary to changing it _now_ without serious thinking.

 
> 
> I have spent a considerable amount of time coming up with this API when I first
> proposed it so, I would be very wary to changing it _now_ without serious
> thinking.
> 

OK, I accept your patch and apologize for changing the behavior without raising it for discussion at the time.

The consensus was to apply Mathieu's "fix bug" patch and close the bug.