tasklwIP - A Lightweight TCP/IP stack - Tasks: task #10370, Raw API: Callback for received FIN...

 
 

You are not allowed to post comments on this tracker with your current authentication level.

task #10370: Raw API: Callback for received FIN to confirm connection is closed

Submitted by:  Iordan Neshev <iordan_neshev>
Submitted on:  Wed 05 May 2010 09:17:12 PM UTC  
 
Category:  None Should Start On:  Wed 05 May 2010 12:00:00 AM UTC
Should be Finished on:  Wed 05 May 2010 12:00:00 AM UTC Priority:  5 - Normal
Status:  Done Privacy:  Public
Percent Complete:  100% Assigned to:  Simon Goldschmidt <goldsimon>
Open/Closed:  Closed Planned Release:  1.4.0
Effort:  50.00

( Jump to the original submission)

Wed 16 Jun 2010 11:00:20 AM UTC, comment #8: 

>Do you mean the 'err' callback?

Yes, I did.

>If so, just set it back to NULL: lwIP's TCP is currently >designed to always call the callbacks as long as the pcb lives, >no matter which state it is in (e.g. when the pcb is removed ?>from TIME_WAIT because the stack ran out of new pcbs, the err >callback will be called, too).


>Besides, for a correct implementation of SO_LINGER, the err >callback must be called after tcp_close if a RST is received


So, if I reset the err callback I will not be able to use SO_LINGER. Currently I don't need it, but who knows what the future brings? I'd better not put needless limitations.
Also, I set a flag there (the err cb) that is tested in many places and currently I don't have the time to revise the whole application to find out what would be the impact of such change. I'll try to find some other approach later. If there are problems, I'll ask lwip_users.

I think this task can be closed now.
Thanks again for your work.

Iordan Neshev <iordan_neshev>
Wed 16 Jun 2010 09:51:03 AM UTC, comment #7: 

> [..]but the abort callback my mislead the application


Do you mean the 'err' callback? If so, just set it back to NULL: lwIP's TCP is currently designed to always call the callbacks as long as the pcb lives, no matter which state it is in (e.g. when the pcb is removed from TIME_WAIT because the stack ran out of new pcbs, the err callback will be called, too).

Besides, for a correct implementation of SO_LINGER, the err callback must be called after tcp_close if a RST is received.

Simon Goldschmidt <goldsimon>
Project AdministratorIn charge of this item.
Wed 16 Jun 2010 08:40:56 AM UTC, comment #6: 

Finally, I found the time to check this.

It works as expected in the normal case.

>- no more rx packets after calling tcp_close,
>- if there is unacked rx data (i.e. the receive window is not >at its full size) or data arrives after we called close: RST is >sent and the connection is aborted


I was not able to test these cases. I'm not quite sure that the connection should be aborted. Sending RST is good, but the abort callback my mislead the application. At the time I call tcp_close/shutdown, the peer's data may be already present on the wire (worst case is GPRS net where the lags are usually more than a second). When I call tcp_close usually I don't care if the peer tells me something more. Probably I'll rise a flag before calling tcp_close/shutdown and if the abort callback fires, I'll ignore the error if this flag is raised.

Just to be clear: I don't want to adapt lwip to my application, but to do what is logical, following the standards. Of course I will adapt my application instead. The problem is that I'm not sure what is the right behavior.

Thanks for fixing this issue!

Iordan Neshev <iordan_neshev>
Tue 18 May 2010 01:12:17 PM UTC, comment #5: 

Sounds good to me Simon - your description matches the behaviour I think we should have.  If Iordan agrees, and ideally can verify this is the behaviour we do have, this can then be closed.

Kieran Mansley <kieranm>
Project Member
Sat 15 May 2010 06:12:35 PM UTC, comment #4: 

That change is nearly three months ago now, but that doesn't matter much.

The actual bug in the changes introduced by task #10088 was the handling of RX packets when TF_RXCLOSED was set: pcb->sent() was still called but ->recv() wasn't.

I changed that so ->recv() is called but the connection is reset when packets including data are received when TF_RXCLOSED is set. This is to inform the remote host that not all the data it sent has been processed (and what is done in tcp_close_shutdown(), too).

So basically, the stack should now behave much the same as 1.3.2 did with these differences:
- no more rx packets after calling tcp_close,
- if there is unacked rx data (i.e. the receive window is not at its full size) or data arrives after we called close: RST is sent and the connection is aborted

This means that we can still call close right after enqueueing all packets and receive the remote host's FIN correctly unless it breaks our assumptions and still sends data.

Receiving data after closing our TX channel can be done by calling tcp_shutdown.

Iordan, could you please verify this works as I expect it to?
Thanks for reporting this!

Simon Goldschmidt <goldsimon>
Project AdministratorIn charge of this item.
Tue 11 May 2010 04:59:16 PM UTC, comment #3: 

I think that this callback should always come up. How else would an application know what happened with its close request?

I think that calling it with p == NULL is the best way to inform the application that the link is closed or going to be closed, as it was a month ago.

Generally (p==NULL) is enough for the application to know that the link is no more usable.

The err argument could reveal more info on what is going on in case we need to know. For example:

ERR_OK - peer ACKed our close request, the link can no longer carry data in either direction. We should clean up and free the PCB (probably outside the callback)

ERR_CLSD - the peer actively closes the connection (FIN), he has nothing more to send to us (while we MAY be able to tell him "good bye" before we close the connection on our side). He expects FIN+ACK from us (does he?).

ERR_ABRT - the peer sent us RST, he aborted the connection on its side. We MUST not send him more data. The application might still be able to get some data which was received before the RST was received.

Such behavior will let older applications work with lwip 1.4.0 with less changes.

By the way, currently the callback is always called with err=ERR_OK.

Iordan Neshev <iordan_neshev>
Tue 11 May 2010 04:34:31 PM UTC, comment #2: 

I think it was meant like that. I'm a bit unceartain if the callback should never come or only if we send a RST (we send a RST if we call close but have unacked data left)...

Simon Goldschmidt <goldsimon>
Project AdministratorIn charge of this item.
Tue 11 May 2010 09:15:48 AM UTC, comment #1: 

Seems like replacing the old tcp_close() with

err = tcp_shutdown(&my_pcb, FALSE, TRUE);
// shut_rx= FALSE - do not close RX, we need to get the peer's FIN;
// shut_tx = TRUE  -  we will not send data any more

does exactly what I wanted.

If this is the right way to do it, please close this task and sorry for the noise. A note in rawapi.txt about this might be useful.

Iordan Neshev <iordan_neshev>
Wed 05 May 2010 09:17:12 PM UTC, original submission:  

Recently the TCP_EVENT_RECV macro was changed  -
it no longer calls the user-defined recv callback when TF_RXCLOSED is set (which is set when we locally close the connection with tcp_close/tcp_shutdown). Now it calls the default callback (tcp_recv_null) instead.

In the near past the (raw) application had to call tcp_close() in order to locally initiate the connection closing. The peer ACKs our close request by sending us FIN. This FIN triggers this:

TCP_EVENT_RECV(pcb, NULL, ERR_OK, err);
in tcp_in.c: tcp_input(), line 346.

In the previous version the user-defined recv callback was called with pbuf p == NULL. That's how the applications were informed that the link is closed by the remote end.

In the new code TF_RXCLOSED is rised when tcp_close is called, preventing the TCP_EVENT_RECV() macro to call the user-defined recv callback.

This functionality is needed in my application - I send files to a FTP server. When I send all the data, I call tcp_close() and I wait for recv callback to fire with p == NULL (i.e. I'm waiting for the FIN). This is the only way for me to be sure that the whole file is uploaded.

There must be a way to inform the application for this event.
If calling the callback with p==NULL is no longer possible, it could be called with err == ERR_CLSD instead.

Currently the macro looks like this:
#define TCP_EVENT_RECV(pcb,p,err,ret)                              \
  do {                                                             \
    if(((pcb)->recv != NULL) && (!((pcb)->flags & TF_RXCLOSED))) { \
      (ret) = (pcb)->recv((pcb)->callback_arg,(pcb),(p),(err));    \
    } else {                                                       \
      (ret) = tcp_recv_null(NULL, (pcb), (p), (err));              \
    }                                                              \
  } while (0)

One possible solution is to make it call
"(ret) = (pcb)->recv((pcb)->callback_arg,(pcb),(p),(err));"
even when the TF_RXCLOSED is rised, by ORing the condition with (err == ERR_CLSD):

#define TCP_EVENT_RECV(pcb,p,err,ret)                              \
  do {                                                             \
    if(((pcb)->recv != NULL) &&                                    \
      ((!((pcb)->flags & TF_RXCLOSED)) || ((err) == ERR_CLSD) )) { \
  (ret) = (pcb)->recv((pcb)->callback_arg,(pcb),(p),(err));    \
    } else {                                                       \
      (ret) = tcp_recv_null(NULL, (pcb), (p), (err));              \
    }                                                              \
  } while (0)

Then if we change
TCP_EVENT_RECV(pcb, NULL, ERR_OK, err); with
TCP_EVENT_RECV(pcb, NULL, ERR_CLSD, err);
the callback will be called with p==NULL and err==ERR_CLSD,
which is enough for me to conclude the peer ACKed my close request.

The proposed macro change has the negative consequence that if there is no user-defined recv callback, then the default recv callback tcp_recv_null() will be called with err==ERR_CLSD instead of ERR_OK, which will lead to a bug (tcp_close() would not be called).

Another, better way is to define this macro:
#define TCP_EVENT_RECV_FIN(pcb)                              \
  do {                                                             \
    if((pcb)->recv != NULL ) { \
  (pcb)->recv((pcb)->callback_arg,(pcb),NULL,ERR_CLSD);    \
    } \
  } while (0)

and call it after the original TCP_EVENT_RECV(pcb, NULL, ERR_OK, err);

The code would look like this:

        /* If a FIN segment was received, we call the callback
           function with a NULL buffer to indicate EOF. */
        if (recv_flags & TF_GOT_FIN) {
          /* correct rcv_wnd as the application won't call tcp_recved()
             for the FIN's seqno */
          if (pcb->rcv_wnd != TCP_WND) {
            pcb->rcv_wnd++;
          }

          TCP_EVENT_RECV(pcb, NULL, ERR_OK, err); // this will call the default recv callback
          TCP_EVENT_RECV_FIN(pcb); // this will call the user-defined callback if one is defined

          if (err == ERR_ABRT) {
            goto aborted;
          }

Note that err is not touched by TCP_EVENT_RECV_FIN(), it stays as
TCP_EVENT_RECV() has left it. This is something that we should discuss later if you accept this approach.

By the way, when the peer closes the connection (i.e. he initiates the close), everything is like before - the recv callback gets called with p==NULL, so my application has no problems when it downloads files.

I'm currently using this new macro as workaround to the introduced problem. So what do you think?

Iordan Neshev <iordan_neshev>

 

No files currently attached

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -email is unavailable- added by kieranm (Posted a comment)
  • -email is unavailable- added by goldsimon (Posted a comment)
  • -email is unavailable- added by iordan_neshev (Submitted the item)
  •  

    Do you think this task is very important?
    If so, you can add your encouragement to it.
    This task has 0 encouragements so far.

    Only logged-in users can vote.

     

     

     

    Follow 6 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2010-06-16 goldsimon StatusReady For Test => Done
        Open/ClosedOpen => Closed
    2010-05-16 goldsimon Planned ReleaseNone => 1.4.0
    2010-05-15 goldsimon StatusNone => Ready For Test
        Percent Complete0% => 100%
        Assigned toNone => goldsimon

    Back to the top


    Powered by Savane 3.5