Re:[lwip-devel] [bug #24830] Socket never closed sometimes.

[Jeff Barber]

Though I'm still a newbie with lwIP, I think I sort of agree with
Jim's leaning but not the detail.  The problem is in the definition of
tcp_close.  I think using tcp_abort() in its place is too extreme;
tcp_abort doesn't allow for an orderly session shutdown.  It simply
sends the RST and blows away all local data structures; it's really
intended for error situations.

TCP has the notion of a "half-closed" connection.  For example, an
HTTP client might send a request to the server, then call shutdown
with SHUT_WR.  This causes the client end to send FIN and the local
connection state moves to FIN_WAIT_1 and once all its own sent data
has been acked, it moves to state FIN_WAIT_2.  From the client's point
of view, it's all done sending.  However, it still wants to receive
and still has an open socket descriptor; it's perfectly okay for the
HTTP server to then send many megabytes of data back to the client
(indeed it need never close).  As I understand it, this is essentially
what lwIP's current tcp_close does.

If the actual intent of tcp_close is to fully close the connection but
in an orderly way (as with a Unix close system call), then it should
do almost the same thing as the current tcp_close but also set a timer
on the FIN_WAIT_2 state.

>From "TCP/IP Illustrated Volume 2", chapter 25, section 25.1, bullet 6
(http://my.safaribooksonline.com/020163354X/ch25):
"A FIN_WAIT_2 timer. When a connection moves from the FIN_WAIT_1 state
to the FIN_WAIT_2 state (Figure 24.15) and the connection cannot
receive any more data (implying the process called close, instead of
taking advantage of TCP's half-close with shutdown), this timer is set
to 10 minutes. When this timer expires it is reset to 75 seconds, and
when it expires the second time the connection is dropped. The purpose
of this timer is to avoid leaving a connection in the FIN_WAIT_2 state
forever, if the other end never sends a FIN. (We don't show this
timeout in Figure 24.15.)"

The current tcp_close behavior is ideal for a tcp_shutdown_write
function (or something like that).  In this case, you would not want
to do anything special with already-buffered received data; this
(along with any subsequently received data) should continue to be
processed ordinarily.

If you want to do a real close (as in closing a Unix socket), then you
should null any receive callbacks and discard any previously-received
data and only keep around the minimal pcb needed to handle the
remaining state transitions.  Any subsequently received data should
just be discarded until the peer's FIN is received or the FIN_WAIT_2
timer expires.

Regards,
Jeff


On Tue, Mar 31, 2009 at 11:27 AM, Jim Pettinato <address@hidden> wrote:
>
> Couldn't the netconn just call tcp_abort in this case?
>
[snip]
> -----Original Message-----
> From: address@hidden
> [mailto:address@hidden On Behalf
> Of Kieran Mansley
> Sent: Tuesday, March 31, 2009 11:03 AM
> To: Kieran Mansley; Homyak; address@hidden
> Subject: [lwip-devel] [bug #24830] Socket never closed sometimes.
>
>
> Follow-up Comment #5, bug #24830 (project lwip):
>
> Now I come to think about how to fix this, there is a minor problem:
> data can be buffered on RX at lots of levels in the stack (within the
> TCP layer, netconn layer, and sockets layer).  Therefore tcp_close()
> needs to know if there are any bytes buffered in the higher layers, but
> it would be hard for it to work this out.  I therefore think adding a
> parameter that gets filtered down through the layers that says "there is
> buffered data in the layer(s) above", and then passed to
> netconn_delete() and tcp_close() would be a good idea.  However, this
> changes the API and so causes problems that way.
>
> It would be fairly simple to alias the existing API functions to the new
> ones, eg:
> #define tcp_close(pcb) tcp_close_new(pcb, 0) #define
> netconn_delete(conn) -> netconn_delete_new(conn, 0) (I would choose
> better names than *_new() - any suggestions?)
>
> This would result in minimal extra code, and keeps the API backwardly
> compatible, but is a bit ugly in the long term.
>
>    _______________________________________________________
>
> Reply to this item at:
>
>  <http://savannah.nongnu.org/bugs/?24830>
>
> _______________________________________________
>  Message sent via/by Savannah
>  http://savannah.nongnu.org/
>
>
>
> _______________________________________________
> lwip-devel mailing list
> address@hidden
> http://lists.nongnu.org/mailman/listinfo/lwip-devel
>
>
> _______________________________________________
> lwip-devel mailing list
> address@hidden
> http://lists.nongnu.org/mailman/listinfo/lwip-devel
>
>