Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

CONNECT_ONLY connections can get closed when cache is full #4369

Closed
loblik opened this issue Sep 17, 2019 · 4 comments
Closed

CONNECT_ONLY connections can get closed when cache is full #4369

loblik opened this issue Sep 17, 2019 · 4 comments

Comments

@loblik
Copy link

@loblik loblik commented Sep 17, 2019

There was a discussion recently about connection cache and CONNECT_ONLY connections. in this issue #3983 (comment)_ . And I think I've just hit the same thing.

I use CONNECT_ONLY to establish the connection and then put filedescriptor in own event loop to do some websocket communication. Recently I've noticed that sometimes connection is closed when it's returned to cache. I think this makes CURLINFO_ACTIVESOCKET unusable since Curl_conncache_return_conn does not consider CONNECT_ONLY connection as in use.

Here an example to reproduce the issue.

#include <curl/curl.h>
#include <unistd.h>
#include <fcntl.h>

void addNewConn(CURLM *multi)
{
    CURL *easyRaw = curl_easy_init();
    curl_easy_setopt(easyRaw, CURLOPT_URL, "https://curl.haxx.se/");
    curl_easy_setopt(easyRaw, CURLOPT_CONNECT_ONLY, 1L);
    curl_easy_setopt(easyRaw, CURLOPT_VERBOSE, 1L);

    curl_multi_add_handle(multi, easyRaw);
}

int main(void)
{

    CURLM *multi_handle = curl_multi_init();

    curl_multi_setopt(multi_handle, CURLMOPT_MAXCONNECTS, 1); 

    addNewConn(multi_handle);

    int still_running, repeats = 0, sockfd;

    for (;;)
    {   
      CURLMcode mc; 

      mc = curl_multi_perform(multi_handle, &still_running);

      if(mc == CURLM_OK ) { 

        struct CURLMsg *m; 

        int msgq = 0;
        m = curl_multi_info_read(multi_handle, &msgq);
        if(m && (m->msg == CURLMSG_DONE)) {
          CURL *e = m->easy_handle;
          if( repeats++)
            break;
          curl_easy_getinfo(m->easy_handle, CURLINFO_ACTIVESOCKET, &sockfd);
          printf("active socket socfd: %d\n", sockfd);
          addNewConn(multi_handle);
        }   

        mc = curl_multi_wait(multi_handle, NULL, 0, 1000, NULL);
      }   
    }   

    if (fcntl(sockfd, F_GETFD) < 0)
    {   
        printf("socfd: %d\n", sockfd);
        perror("fcntl");
    }   

    return 0;
}
@bagder
Copy link
Member

@bagder bagder commented Dec 9, 2019

I don't see how this is a bug. This seems to work exactly as intended:

  1. You limit libcurl to only keep one connection alive.
  2. You issue a request that sets up a connect-only request , performs that and extracts the socket
  3. You then create a new request, that cannot reuse the first connection and that runs.
  4. When the second request completes, it puts it back in the connection cache, finds that it needs to "kill off" one to fit it within the set limit. It then closes the oldest.

Where is the error here?

@loblik
Copy link
Author

@loblik loblik commented Dec 10, 2019

I have just set the cache size to one to make this easier to reproduce. But I've experienced this even with default setting. It behaves exactly as you described. The problem is the CONNECT_ONLY connections with extracted socket using CURLINFO_ACTIVESOCKET probably should not be closed (until curl_easy_cleanup is called). As this makes extracted socket unusable.

@bagder
Copy link
Member

@bagder bagder commented Dec 10, 2019

The error here is probably that the connection should be treated as in use and thus shouldn't be considered for closing.

bagder added a commit that referenced this issue Dec 10, 2019
This makes them never to be considered "the oldest" to be discarded when
reaching the connection cache limit. The reasoning here is that
CONNECT_ONLY is primarily used in combination with using the
connection's socket post connect and since that is used outside of
curl's knowledge we must assume that it is in use until explicitly
closed.

Reported-by: Pavel Pavlov
Reported-by: Pavel Löbl
Fixes #4426
Fixes #4369
@loblik
Copy link
Author

@loblik loblik commented Dec 10, 2019

Yes. I cannot reproduce it with that change anymore. Thanks

@bagder bagder closed this in 1d5c427 Dec 11, 2019
@lock lock bot locked as resolved and limited conversation to collaborators Mar 10, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

3 participants
You can’t perform that action at this time.