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

Submitted by:  TabascoEye <tabascoeye>
Submitted on:  Thu 14 Jan 2010 11:21:30 AM UTC  
 
Category: TCPSeverity: 3 - Normal
Item Group: Faulty BehaviourStatus: Fixed
Privacy: PublicAssigned to: Simon Goldschmidt <goldsimon>
Open/Closed: ClosedPlanned Release: None
lwIP version: Other

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

Checked in as noon disagreed...

Simon Goldschmidt <goldsimon>
Project AdministratorIn charge of this item.
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>
Project AdministratorIn charge of this item.
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>
Project AdministratorIn charge of this item.
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>
Project AdministratorIn charge of this item.
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>

 

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
  • -unavailable- added by goldsimon (Posted a comment)
  • -unavailable- added by tabascoeye (Submitted the item)
  •  

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

    Only logged-in users can vote.

     

    Please enter the title of George Orwell's famous dystopian book (it's a date):

     

     

    Follow 6 latest changes.

    Date Changed By Updated Field Previous Value => Replaced By
    Wed 27 Jan 2010 06:26:49 PM UTCgoldsimonOpen/ClosedOpen=>Closed
    Wed 27 Jan 2010 06:26:42 PM UTCgoldsimonStatusIn Progress=>Fixed
    Sat 23 Jan 2010 02:53:12 PM UTCgoldsimonStatusNone=>In Progress
      Assigned toNone=>goldsimon
    Sat 23 Jan 2010 02:52:57 PM UTCgoldsimonAttached File-=>Added lwip_bug_28605.patch, #19537
    Fri 15 Jan 2010 01:20:46 PM UTCtabascoeyeAttached File-=>Added lwip.patch, #19493

    Back to the top


    Powered by Savane 3.1-cleanup1