buglwIP - A Lightweight TCP/IP stack - Bugs: bug #23119, After a read with timeout, the...

 
 

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

bug #23119: After a read with timeout, the socket is unuseable

Submitted by:  Robert Reither <nathan2225>
Submitted on:  Wed 30 Apr 2008 01:38:32 PM UTC  
 
Category: sockets/netconnSeverity: 3 - Normal
Item Group: Change RequestStatus: Fixed
Privacy: PublicAssigned to: Jonathan Larmour <jifl>
Open/Closed: ClosedPlanned Release: None
lwIP version: None

(Jump to the original submission Jump to the original submission)

Fri 09 May 2008 12:15:04 PM UTC, comment #13:

I've checked the patch in now.

Jonathan Larmour <jifl>
Project MemberIn charge of this item.
Sun 04 May 2008 07:46:09 PM UTC, comment #12:

Ok, it's fine for me. I will do more tests tomorrow with some tcp apps, but it's ok for contrib/apps modules (based only on udp/raw).

Frédéric Bernon <fbernon>
Project Member
Thu 01 May 2008 06:02:33 PM UTC, comment #11:

> .. the define of fatal or minor codes was done on a IRC chat with Simon and me in november


Ah, I didn't remember that. That's the reason noone else but us knows about it ;-)

The patch looks fine to me, I just can't really test it right now (but I think I tested it like that in november)

Simon Goldschmidt <goldsimon>
Project Administrator
Thu 01 May 2008 03:34:37 PM UTC, comment #10:

Updated patch as per Frederic's comment #9. Frederic says he'll test it over the weekend, so all being well, I'll check in on Monday.

(file #15576)

Jonathan Larmour <jifl>
Project MemberIn charge of this item.
Thu 01 May 2008 02:47:51 PM UTC, comment #9:

hi,

Sorry, I'm not at the office (and I can't access to my files), but the define of fatal or minor codes was done on a IRC chat with Simon and me in november (if someone can access to the lwip irc channel archives), but was done for "task #6969 : Review usage of conn->err in netconn layer". For memory, I said to Simon that most of stacks I used "dead" a tcp connection on a timeout (like tcp keepalive, but faster). But SO_RCVTIMEO doesn't have really a big portability, so, if there is a consensus to change the current behavior, I don't have any objections to change it (just, test it with contrib/apps if possible, else, I will do it this weekend).

Jonathan, just don't forget to update err_to_errno_table in sockets.c (it is not in your patch).

Frédéric Bernon <fbernon>
Project Member
Thu 01 May 2008 02:32:20 PM UTC, comment #8:

I am using the api calls and things are working as expected using the supplied patch. The previous timeout error doesn't error out netconn_write() anymore.

Thanks,

Dustin

Dustin Stahlback <dstahlback>
Thu 01 May 2008 01:56:22 PM UTC, comment #7:

Dustin, actually the patch I'm attaching should do the work. Is someone prepared to test it?

Simon, I'd say we should just go ahead anyway if no-one else pipes up. I've searched my old email and can't find anything in line with what you remember (which may just mean I didn't get email about it that's all, of course :-)). The change was implemented in patch #5959 for what it's worth.

(file #15571)

Jonathan Larmour <jifl>
Project MemberIn charge of this item.
Thu 01 May 2008 01:31:57 PM UTC, comment #6:

This issue has affected me also, is the fix to have ERR_IS_FATAL act as :

#define ERR_IS_FATAL(e) (((e) < ERR_RTE) || ((e) == ERR_TIMEOUT))

and what resetting is done in lwip_recvfrom() ?

Thanks,

Dustin

Dustin Stahlback <dstahlback>
Thu 01 May 2008 11:57:58 AM UTC, comment #5:

Looks good, declaring this error as NON_FATAL and resetting
the error in socket.c/revfrom()

Can confirm, it looks like as long as only a single thread uses the socket it shouldn't be 'really' blocked.

Robert Reither <nathan2225>
Thu 01 May 2008 08:12:10 AM UTC, comment #4:

Read timeouts should not be fatal.

Kieran Mansley <kieranm>
Project Administrator
Wed 30 Apr 2008 08:08:18 PM UTC, comment #3:

I think the only change should be in err.h, where ERR_TIMEOUT must be move upwards a bit, so that the ERR_IS_FATAL check leads to 0 for ERR_TIMEOUT instead of 1 as it is now.

I don't remember exactly who was saying what, but I guess Frédéric said something which is why I'd like to hear what he has to say about this before changing anything.

Simon Goldschmidt <goldsimon>
Project Administrator
Wed 30 Apr 2008 05:18:24 PM UTC, comment #2:

I don't remember the discussion, so for all I know this is contrary to anything I may or may not have said before :-), but as described in this bug, the timeout should not be fatal.

A grep shows that ERR_TIMEOUT is only used for receive timeouts, so should be safe to treat specially.

If you go with the principle that sockets should not be operated on by more than one thread at one time, then the only change need be in lwip_recvfrom I guess.

Jonathan Larmour <jifl>
Project MemberIn charge of this item.
Wed 30 Apr 2008 04:13:26 PM UTC, comment #1:

Oh, look, someone is actually using the socket read timeout ;-)

The thing is, we discussed this a while ago and if I remember correctly, found a minority for treating timeouts as 'fatal' errors (in contrast to 'non-fatal' errors like out-of-memory or a routing error).

However, since I'm personally no convinced about that solution (I think a timeout should not be fatal and another read after a timeout should be allowed), I'll leave this open and let the others comment ;-)

Simon Goldschmidt <goldsimon>
Project Administrator
Wed 30 Apr 2008 01:38:32 PM UTC, original submission:

If i read from a socket and got a timeout back
(compiled with LWIP_SO_RCVTIMEO and timeout used)

netconn_recv() sets

conn->err = ERR_TIMEOUT.

From now on i cannot send over this socket anymore.

If i reset the conn->err to ERR_OK, its possible to send again.
Somehow this error shouldn't be delivered over
conn->err

because it can block the whole socket meanwhile this way, even if i reset the conn->err after reading the err (and setting errno) in socket.c

Greetings
Robert

Robert Reither <nathan2225>

 

Attached Files
file #15576:  lwip.bug23119.2.patch added by jifl (6KiB - text/x-patch)

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -unavailable- added by fbernon (Posted a comment)
  • -unavailable- added by dstahlback (Posted a comment)
  • -unavailable- added by kieranm (Posted a comment)
  • -unavailable- added by jifl (Posted a comment)
  • -unavailable- added by goldsimon (Posted a comment)
  • -unavailable- added by nathan2225 (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 8 latest changes.

    Date Changed By Updated Field Previous Value => Replaced By
    Fri 09 May 2008 12:15:04 PM UTCjiflStatusConfirmed=>Fixed
      Open/ClosedOpen=>Closed
    Thu 01 May 2008 03:34:36 PM UTCjiflAttached File-=>Added lwip.bug23119.2.patch, #15576
    Thu 01 May 2008 03:33:13 PM UTCjiflAttached File#15571=>Removed
    Thu 01 May 2008 01:56:22 PM UTCjiflAttached File-=>Added lwip.bug23119.patch, #15571
      Assigned toNone=>jifl
    Wed 30 Apr 2008 04:13:26 PM UTCgoldsimonItem GroupNone=>Change Request
      StatusNone=>Confirmed

    Back to the top


    Powered by Savane 3.1-cleanup1