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

Submitter:  Robert Reither <nathan2225>
Submitted:  Wed 30 Apr 2008 01:38:32 PM UTC
   
 
Category:  sockets/netconn Severity:  3 - Normal
Item Group:  Change Request Status:  Fixed
Privacy:  Public Assigned to:  jifl
Open/Closed:  Closed Planned Release:  None
lwIP version:  None

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>
Group Member
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>
Group 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>
Group 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>
Group Member
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>
Group 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>
Group Member
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>
Group Member
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>
Group 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>
Group Member
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>
Group 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>

 

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

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

    Date Changed by Updated Field Previous Value => Replaced by
    2008-05-09 jifl StatusConfirmed Fixed
        Open/ClosedOpen Closed
    2008-05-01 jifl Attached File- Added lwip.bug23119.2.patch, #15576
    2008-05-01 jifl Attached File#15571 Removed
    2008-05-01 jifl Attached File- Added lwip.bug23119.patch, #15571
        Assigned toNone jifl
    2008-04-30 goldsimon Item GroupNone Change Request
        StatusNone Confirmed

    Back to the top

    Powered by Savane 3.12