buglwIP - A Lightweight TCP/IP stack - Bugs: bug #26069, Documentation of tcp_connect()...

 
 

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

bug #26069: Documentation of tcp_connect() incorrectly describes handling of errors

Submitted by:  Rikke Ottesen <ro03>
Submitted on:  Thu 02 Apr 2009 01:42:00 PM UTC  
 
Category:  TCP Severity:  3 - Normal
Item Group:  Faulty Behaviour Status:  Fixed
Privacy:  Public Assigned to:  Simon Goldschmidt <goldsimon>
Open/Closed:  Closed Planned Release:  None
lwIP version:  1.3.0

( Jump to the original submission)

Mon 02 Jun 2014 09:27:44 PM UTC, comment #19: 

OK, fixed the wiki and will keep checking for err == ERR_OK in tcp_connect callback. THX!

Sergio R. Caprile <scaprile>
Mon 02 Jun 2014 07:58:29 PM UTC, comment #18: 

The comment in the source code can be easily fixed, but I wouldn't omit the erro checking in connect callbacks: the parameter is there and we cannot remove it without breaking every code ever writes for lwIP. Plus the err parameter could be != ERR_OK sooner or later (e.g. on error if the err callback is not set).

Simon Goldschmidt <goldsimon>
Project AdministratorIn charge of this item.
Mon 02 Jun 2014 05:25:18 PM UTC, comment #17: 

Hi Simon, 5 years later I have to say "not so trivial".
I found these in 1.4.1:
- The comment in the source code for tcp_connect() also states that the connect callback will be called, either for good or for bad.
- The code for the tcp_connect callback in SMTP.c checks err when called, reinforcing the idea.
- The wiki also states that the connect callback will be called either connected or not.

After following these, I was surprised when my err callback was always called on RST or timeout.

I can handle the wiki, the others are out of my reach.

Sergio R. Caprile <scaprile>
Tue 21 Apr 2009 07:17:24 PM UTC, comment #16: 

Since I think this is trivial, I just updated rawapi.txt (~> "if connect fails, pcb->err is called")

Simon Goldschmidt <goldsimon>
Project AdministratorIn charge of this item.
Mon 06 Apr 2009 08:44:15 AM UTC, comment #15: 

Hello,
this is a bit of my experience, I hope
it will help:

First I allocate a socket descriptor, associated
with a static application buffer (not a pbuf):

sockfd_t cb_sock(...) // allocate a socket
{
    ... ...
    tcp_recv(cb_Sockets[sockfd].pcb, CB_active_recv);
    tcp_err (cb_Sockets[sockfd].pcb, CB_errhanler);
}

int cb_connect(int sockfd,  struct sockaddr_in *ptr_sockaddr_remote)
{
    ....
    err = tcp_connect(cb_Sockets[sockfd].pcb, &remote_ipaddr,
            port, CB_active_connected);
    ....
}

According to rawapi.h CB_active_connected() should be
called with (err != ERR_OK). In my application this
never happens. The error handler (CB_errhanler) is
called instead. For me it just sets one of the higher-level
application error flags. I check it everytime I use
the socket to send or receive data.

Offtopic:
Simply, depending on the place
where whatever problem is detected a different type of
flags are rised, eventually tcp_close() and/or tcp_abort()
may be called. The poll handler clears any garbage after
some timeout if needed, allowing the application to get any
buffered received data. The process is much complicated for
me since it must handle also other events such as
remote close requests and local close or shutdown request.

Keep in mind that "The error callback function does not get the pcb passed to it as a parameter since the pcb may already have been deallocated. ..."

I use the "almost" latest version of lwip - about a month ago.

Greetings,
Iordan

Iordan Neshev <iordan_neshev>
Fri 03 Apr 2009 10:42:02 AM UTC, comment #14: 

Ok fine, just to be sure :)

Rikke Ottesen <ro03>
Fri 03 Apr 2009 10:19:10 AM UTC, comment #13: 

I'm going to change the documentation.

If we were starting from scratch, there's a case for calling TCP_EVENT_CONNECTED, but changing this would break all the existing users of the raw API, including the netconn and socket layers included in lwIP, which is something I want to avoid.

Although it's odd that you wait on TCP_EVENT_CONNECTED for success and TCP_EVENT_ERR for failure, it would also be odd that you wait on TCP_EVENT_CONNECTED for connection errors, and TCP_EVENT_ERR for all other errors.

Kieran Mansley <kieranm>
Project Member
Fri 03 Apr 2009 10:03:25 AM UTC, comment #12: 

Just to be clear, are you going to change the code so it calls the TCP_EVENT_CONNECTED in this state, or are you going to change documentaion?

I just think it seems weird that you wait for a TCP_EVENT_CONNECTED on a success criteria but on a failure you wait for a  TCP_EVENT_ERR, or what?

Rikke Ottesen <ro03>
Fri 03 Apr 2009 09:28:12 AM UTC, comment #11: 

I can see how the documentation is confusing, it should be corrected.

I've updated the summary of the bug to reflect this.

Kieran Mansley <kieranm>
Project Member
Fri 03 Apr 2009 09:06:19 AM UTC, comment #10: 

As the lwip documentation state:

"If the connection could not be properly established, either because the other host refused the connection or because the other host did'nt answer, the "connected" function will be called with the "err" argument set accordingly.

So I think you in this particular state should send  TCP_EVENT_CONNECTED in stead, as we use this API documentation.

Rikke Ottesen <ro03>
Fri 03 Apr 2009 09:01:31 AM UTC, comment #9: 

It will get there once pcb->ntrx increases to TCP_SYNMAXRTX.  Note this can take some time to happen, as retransmissions back off exponentially.  It may be that you're just not waiting long enough, and that setting TCP_SYNMAXRTX to a smaller value would help.

The only question on this bug is whether TCP_EVENT_CONNECTED should be called when the connection fails, or whether TCP_EVENT_ERR should be called instead.  I'm inclined to stick with TCP_EVENT_ERR as that is the existing behaviour and changing it to TCP_EVENT_CONNECTED might cause confusion for other users of the event API.

Kieran Mansley <kieranm>
Project Member
Fri 03 Apr 2009 08:52:03 AM UTC, comment #8: 

Yes, but the problem is that the pcb->state is SYN_SENT, and this is not checked.
I have added some code to make that check

    if (pcb->state == SYN_SENT) {
      if ((u32_t)(tcp_ticks - pcb->tmr) > 10) {
        ++pcb_remove;
        LWIP_DEBUGF(TCP_DEBUG, ("tcp_slowtmr: removing pcb stuck in SYN_SENT\n"));
      }
    }

Otherwise it will never go into the
/* If the PCB should be removed, do it. */
and call
 TCP_EVENT_ERR(pcb->errf, pcb->callback_arg, ERR_ABRT);

That is the main problem.

Rikke Ottesen <ro03>
Fri 03 Apr 2009 08:34:31 AM UTC, comment #7: 

If we get failure in tcp_slowtmr() would be set flag pcb_remove.
Near to the end of the loop we check this variable and send TCP_EVENT_ERR.

/* If the PCB should be removed, do it. */
if (pcb_remove) {
...
  TCP_EVENT_ERR(pcb->errf, pcb->callback_arg, ERR_ABRT);
}

TCP_EVENT_ERR is not TCP_EVENT_CONNECTED with error code,
but good enough too:)

Oleg Tyshev <olegreen>
Fri 03 Apr 2009 08:29:28 AM UTC, comment #6: 

If you fix this problem in some other way, than the newest attached file I gave you, could you please let me know? Then I'll add this fix in stead of my own. That would be great!

Cheers
Rikke

Rikke Ottesen <ro03>
Fri 03 Apr 2009 08:25:42 AM UTC, comment #5: 

Yes, I think so.

Kieran Mansley <kieranm>
Project Member
Fri 03 Apr 2009 08:13:30 AM UTC, comment #4: 

Im not that into the pcb->nrtx part, but one thing I do no is that the TCP_EVENT_CONNECTED() function is never called.
I have looked through all the code and the TCP_EVENT_CONNECTED() is ONLY called when the connection is a success.
It is called in:

 static err_t tcp_process(struct tcp_pcb *pcb);

So even though I wait a long time I never get a failure reply to the App.

I expect as the documentation says, that a failure should be retrieved when the connection cannot be established.

The point is that the TCP_EVENT_CONNECTED() should be called somewhere if the connection is never established, the question is where it should be called.
I have looked on the site http://en.wikipedia.org/wiki/File:Tcp_state_diagram_fixed.svg as I see it the pcb should be closed/freed if the connection is not established, and the TCP_EVENT_CONNECTED() should be called with a failure.

Does this make sense? :)

Rikke Ottesen <ro03>
Thu 02 Apr 2009 02:50:56 PM UTC, comment #3: 

pcb->nrtx should eventually get to TCP_SYNMAXRTX, as pcb->ntrx is increased each time the SYN is retransmitted, so the suggested fragment should I think deal with detecting a duff connection.  Are you saying that pcb->nrtx isn't getting increased properly, or perhaps that it is somehow greater than TCP_SYNMAXRTX?

If tcp_slowtmr() is removing the connection and the application still isn't noticing (due to TCP_EVENT_CONNECTED()) not being called, that is something we need to fix.

Kieran Mansley <kieranm>
Project Member
Thu 02 Apr 2009 02:34:25 PM UTC, comment #2: 

I thought so to, but the pcb->nrtx == TCP_SYNMAXRTX is not true, so it does not go into this case.

Another thing is that the TCP_EVENT_CONNECTED()isn't called with the error code, so the Application does not receive the failure on the connect and keeps hanging because of no reply.

Rikke Ottesen <ro03>
Thu 02 Apr 2009 02:15:58 PM UTC, comment #1: 

Isn't this case covered by the lines (around tcp.c:583 in CVS head) in tcp_slowtmr():

    if (pcb->state == SYN_SENT && pcb->nrtx == TCP_SYNMAXRTX) {
      ++pcb_remove;

Kieran Mansley <kieranm>
Project Member
Thu 02 Apr 2009 01:42:00 PM UTC, original submission:  

Problem:
Calling tcp_connect() with a remote IP address which is not present;
The pcb->state = SYN_SENT

The connection never succeeds, because the remote side is not present.

Resolution:
The tcp_slowtmr(void) function should check on the SYN_SENT state and after a while ....

    if (pcb->state == SYN_SENT) {
      if ((u32_t)(tcp_ticks - pcb->tmr) > 10) {
        ++pcb_remove;
        LWIP_DEBUGF(TCP_DEBUG, ("tcp_slowtmr: removing pcb stuck in SYN_SENT\n"));
      }
    }

...remove the PCB and call the callback function  with an error TCP_EVENT_CONNECTED(). Otherwise the APP whil hang!

I have added the changes I made. Search for CSR begin and you'll find the code added. The number 10 I have hardcoded should of cause be changed. This was just for testing.

Rikke Ottesen <ro03>

 

Attached Files
file #17850:  tcp.c added by ro03 (43KiB - text/plain - A newer file submittet....you either call the TCP_EVENT_CONNECTED if it is SYN_SENT state else you call the TCP_EVENT_ERR callback function)
file #17849:  tcp.c added by ro03 (43KiB - text/plain)

 

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 iordan_neshev (Posted a comment)
  • -email is unavailable- added by olegreen (Posted a comment)
  • -email is unavailable- added by kieranm (Posted a comment)
  • -email is unavailable- added by ro03 (Submitted the item)
  •  

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

    Only logged-in users can vote.

     

     

     

    Follow 11 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2014-06-04 goldsimon StatusIn Progress => Fixed
        Open/ClosedOpen => Closed
    2014-06-02 goldsimon StatusFixed => In Progress
        Open/ClosedClosed => Open
    2009-04-21 goldsimon StatusNone => Fixed
        Assigned toNone => goldsimon
        Open/ClosedOpen => Closed
    2009-04-03 kieranm Planned Release => 1.3.1
    2009-04-03 kieranm SummaryConnect to a remote IP address which is not present makes the APP hang => Documentation of tcp_connect() incorrectly describes handling of errors
    2009-04-02 ro03 Attached File- => Added tcp.c, #17850
    2009-04-02 ro03 Attached File- => Added tcp.c, #17849

    Back to the top


    Powered by Savane 3.5