buglwIP - A Lightweight TCP/IP stack - Bugs: bug #28605, Write can block when cable...

 
 

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

bug #28605: Write can block when cable unplugged

Submitter:  TabascoEye <tabascoeye>
Submitted:  Thu 14 Jan 2010 11:21:30 AM UTC
   
 
Category:  TCP Severity:  3 - Normal
Item Group:  Faulty Behaviour Status:  Fixed
Privacy:  Public Assigned to:  goldsimon
Open/Closed:  Closed Planned Release:  None
lwIP version:  Other

Wed 27 Jan 2010 06:26:42 PM UTC, comment #5: 

Checked in as noon disagreed...

Simon Goldschmidt <goldsimon>
Group administrator
Sat 23 Jan 2010 02:52:57 PM UTC, comment #4: 

Since the '+2' was not really satisfying, I've created a patch (against CVS head), that adds an option TCP_SNDQUEUELOWAT much like TCP_SNDLOWAT but for pcb->snd_queuelen.

Could you verify it works for you?

(file #19537)

Simon Goldschmidt <goldsimon>
Group administrator
Fri 15 Jan 2010 04:54:57 PM UTC, comment #3: 

Oh, now I see what you are getting at. So your change only makes sense when using select() to see which socket is writable? (I didn't get that from the initial post of the summary).

Anyway, I guess the patch is good except for the '+ 1' when comparing send_queuelen: Currently it's OK for sockets since PBUF_RAM is used for sending data on tcp sockets, but this might change in the future (moving from PBUF_RAM as a contiguous buffer to a pbuf chain). For netconns, we might have to change it to '+ 2' since someone might write no-copy data, in which case 2 pbufs are used (1 _RAM for header and 1 _REF for data).

Simon Goldschmidt <goldsimon>
Group administrator
Fri 15 Jan 2010 01:09:37 PM UTC, comment #2: 

This scenario is basically an extension of the SNDLOWAT scenario. And in both cases we want to send the APIMSG to signal to select() that the write_FD should be reset because the next write() will block.

In that way it makes the select() a little better because it also reacts on the sendqlen being full before it actually happens.

Still we don't want to really fail here because it still is a temporary situation. when the cable is replugged and packets go out again (and get acked), the sndqlen as well as the buffersize will drop and set the write_FD again so that select() will show the socket writeable again.

The timeout I also want to implement here (as discussed on the mailinglist) is a different scenario that should really fail. More on that on the ML until I have an implementation ready to submit in the patchmanager here

btw: Version Number is 1.3.2-stable as one could deduce from my PS comment :o)

TabascoEye <tabascoeye>
Thu 14 Jan 2010 08:14:25 PM UTC, comment #1: 

Could you go a bit more in detail why calling the API_EVENT in that case helps? I would have thought we might want to abort the write with 'failed' in this case, but that's kind of risky when part of the data has already been sent...

And what do you mean by 'make the socket_select() a little less bumpy '?

Oh, and when giving line numbers, it's always a good idea to include the version of the file, as currently, line 1015 is a comment line... :-)

Simon Goldschmidt <goldsimon>
Group administrator
Thu 14 Jan 2010 11:21:30 AM UTC, original submission:  

When doing netconn_write, the codeflow goes through do_write, do_writemore, tcp_write, tcp_enqueue.

tcp_enqueue checks for the queuelen to be smaller than TCP_SND_QUEUELEN and returns ERR_MEM if it is bigger.
BUT tcp_write() does not care with a comment stating that this is a temporary error.
That is correct when tcp_output() is able to send stuff but without incoming ACKs this might take forever which will block netconn_write() forever.

This can be fixed by early checking of TCP_SND_QUEUELEN and signalling that it will be reached by the next write.
This should also make the socket_select() a little less bumpy

In api_msg.c, function do_writemore(), line 1015:
...
    if ((err == ERR_OK) && ((tcp_sndbuf(conn->pcb.tcp) <= TCP_SNDLOWAT) || ((tcp_sndqlen(conn->pcb.tcp) + 1 >= TCP_SND_QUEUELEN)))) {
      API_EVENT(conn, NETCONN_EVT_SENDMINUS, len);
    }
...

and in tcp.h, line 80/81:

...
#define          tcp_sndqlen(pcb)   ((pcb)->snd_queuelen)
...

Sorry I can't produce an ordinary diff/patch right now, but it's only two lines change.

--
PS: the lwip version field in the savannah Bug reporter misses Version 1.3.2

TabascoEye <tabascoeye>

 

(Note: upload size limit is set to 16384 kB, after insertion of the required escape characters.)

Attached Files
file #19537:  lwip_bug_28605.patch added by goldsimon (5KiB - text/x-patch)
file #19493:  lwip.patch added by tabascoeye (1KiB - application/octet-stream - the two linechanges as patch output)

 

Depends on the following items: None found

Items that depend on this one: None found

 

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

    There are 0 votes so far. Votes easily highlight which items people would like to see resolved in priority, independently of the priority of the item set by tracker managers.

     

    Follow 6 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2010-01-27 goldsimon Open/ClosedOpen Closed
    2010-01-27 goldsimon StatusIn Progress Fixed
    2010-01-23 goldsimon StatusNone In Progress
        Assigned toNone goldsimon
    2010-01-23 goldsimon Attached File- Added lwip_bug_28605.patch, #19537
    2010-01-15 tabascoeye Attached File- Added lwip.patch, #19493

    Back to the top

    Powered by Savane 3.13-02a9.
    Corresponding source code