Navigation Menu

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

Jump to bottom

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
Labels
connecting & proxies needs-info

Comments

@loblik
Copy link

loblik commented Sep 17, 2019 •
edited

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;
}
@loblik loblik closed this as completed Sep 17, 2019
@loblik loblik reopened this Sep 17, 2019
@bagder bagder mentioned this issue Sep 26, 2019
Closed
@bagder
Copy link
Member

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 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 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
@bagder
conncache: CONNECT_ONLY connections are now assumed in-use always
46c2c98 
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
@bagder bagder mentioned this issue Dec 10, 2019
Closed
@loblik
Copy link
Author

loblik commented Dec 10, 2019

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

@bagder bagder closed this as completed 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.
Assignees
No one assigned
Labels
connecting & proxies needs-info
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

conncache: CONNECT_ONLY connections assumed always in-use curl/curl
3 participants
@bagder @jay @loblik