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

curl_multi_fdset does not return the cpool sockets #15156

Closed
chrisd-work opened this issue Oct 4, 2024 · 20 comments
Closed

curl_multi_fdset does not return the cpool sockets #15156

chrisd-work opened this issue Oct 4, 2024 · 20 comments

Comments

@chrisd-work
Copy link
Contributor

chrisd-work commented Oct 4, 2024

I did this

While implementing #15155 I noticed that these two functions do not return the same set of sockets. Notably curl_multi_waitfds can return more than curl_multi_fdset.

I expected the following

I expected these functions to behave in the same way except one is fd_set based, and the other is poll like.

The difference is both functions return the sockets on the list multi->process. However, only curl_multi_waitfds returns the sockets associated with multi->cpool.

I would expect these functions to operate the same way. I don't know the code base well enough to know which is correct.

curl/libcurl version

8.10.1, master

operating system

Ubuntu 24.04

@bagder bagder changed the title Incosistency between curl_multi_fdset and curl_mutli_waitfds Inconsistency between curl_multi_fdset and curl_mutli_waitfds Oct 9, 2024
@chrisd-work
Copy link
Contributor Author

@bagder do you know which version is correct?

@curl curl deleted a comment Oct 28, 2024
@bagder
Copy link
Member

bagder commented Jan 7, 2025

Fixed by #15155 (which used a "fixes" of the wrong issue number)

@bagder bagder closed this as completed Jan 7, 2025
@chrisd-work
Copy link
Contributor Author

I think this was closed in error this was a different issue, I uncovered while working on #15155

@jay
Copy link
Member

jay commented Jan 17, 2025

While implementing #15155 I noticed that these two functions do not return the same set of sockets. Notably curl_mutli_waitfds can return more than curl_multi_fdset.

...

The difference is both functions return the sockets on the list multi->process. However, only curl_multi_waitfds returns the sockets associated with multi->cpool.

I don't know the answer to this and I haven't found any context in the commit message or PR as to why curl_multi_waitfds was added as an alternative to curl_multi_fdset. The documentation does not cover this. @bagder @dkarpov1970

@jay jay reopened this Jan 17, 2025
@bagder
Copy link
Member

bagder commented Jan 17, 2025

Sorry, I thought it was obvious.

curl_multi_fdset creates fd_set collections which are used for select() calls. select() has numerous problems and one easy way to avoid some of its problems is to use poll().

curl_multi_waitfds exports the file descriptors so you can poll them, which curl_multi_fdset does not.

@bagder
Copy link
Member

bagder commented Jan 17, 2025

I mean, this does not explain the bug here. A bug is a bug.

@bagder bagder changed the title Inconsistency between curl_multi_fdset and curl_mutli_waitfds curl_multi_fdset does not return the cpool sockets Jan 17, 2025
@chrisd-work
Copy link
Contributor Author

chrisd-work commented Jan 17, 2025

curl_multi_fdset creates fd_set collections which are used for select() calls. select() has numerous problems and one easy way to avoid some of its problems is to use poll().

curl_multi_waitfds exports the file descriptors so you can poll them, which curl_multi_fdset does not.

I get that, and my concern has nothing to do with it. Users should avoid fdset if they can because of all the issues. However the functions should be consistent aside from the limits that fdset presents.

I have included the relevant code below.
Both versions loop through the multi->process list and add all of the fds from that list. However if you look at the implementation for curl_multi_waitfds, it also adds the fd from multi->cpool. So the two implementations return a different list of fds if multi->cpool is a valid fd.

Relevant code for curl_multi_fdset:

    for(i = 0; i < data->last_poll.num; i++) {
      if(!FDSET_SOCK(data->last_poll.sockets[i]))
        /* pretend it does not exist */
        continue;
#if defined(__DJGPP__)
#pragma GCC diagnostic push
#pragma GCC diagnostic ignored "-Warith-conversion"
#endif
      if(data->last_poll.actions[i] & CURL_POLL_IN)
        FD_SET(data->last_poll.sockets[i], read_fd_set);
      if(data->last_poll.actions[i] & CURL_POLL_OUT)
        FD_SET(data->last_poll.sockets[i], write_fd_set);
#if defined(__DJGPP__)
#pragma GCC diagnostic pop
#endif
      if((int)data->last_poll.sockets[i] > this_max_fd)
        this_max_fd = (int)data->last_poll.sockets[i];
    }
  }

Relevant code for curl_multi_waitfds:

  Curl_waitfds_init(&cwfds, ufds, size);
  for(e = Curl_llist_head(&multi->process); e; e = Curl_node_next(e)) {
    struct Curl_easy *data = Curl_node_elem(e);
    multi_getsock(data, &data->last_poll);
    need += Curl_waitfds_add_ps(&cwfds, &data->last_poll);
  }

  need += Curl_cpool_add_waitfds(&multi->cpool, &cwfds);

@bagder
Copy link
Member

bagder commented Jan 17, 2025

Hence me renaming the issue to make it clear what it is about.

@chrisd-work
Copy link
Contributor Author

Sorry I misunderstood your response to @jay. It looks like the agreement is that curl_multi_fdset should return the cpool fd as well. Thanks.

@jay
Copy link
Member

jay commented Jan 18, 2025

I think the more interesting question is curl_multi_waitfds does rather than curl_multi_fdset does not. Why'd he do it that way for connections not in use for a transfer? I could imagine a scenario where a cpool socket is readable and so poll returns immediately, but is curl_multi_perform actually reading that connection for the data if it's not part of a transfer? Then what if it's a busy loop, curl_multi_waitfds -> poll -> curl_multi_perform all return immediately. Could that happen? @icing

@bagder
Copy link
Member

bagder commented Jan 18, 2025

It's a bug. They should extract the exact same set of sockets + actions.

@jay
Copy link
Member

jay commented Jan 18, 2025

I assume Dmitry had some good reason for doing it that way because it doesn't look accidental.

@bagder
Copy link
Member

bagder commented Jan 18, 2025

Why should they work differently?

@jay
Copy link
Member

jay commented Jan 18, 2025

Oh they shouldn't... I'd still like to hear from him though!

@bagder
Copy link
Member

bagder commented Jan 18, 2025

I wouldn't mind that either, but that doesn't change the fact that they should both return the same sockets in the same states, just different ways.

@jay
Copy link
Member

jay commented Jan 18, 2025

Yes but which way, otherwise we could just write a PR and be done with it. I think it deserves some input and as I said earlier I couldn't find any background on it. IMO it makes more sense not to provide cpool sockets to the caller.

@bagder
Copy link
Member

bagder commented Jan 18, 2025

The shutdown connections should be included

@jay
Copy link
Member

jay commented Jan 19, 2025

That's a good point, I didn't think of that. Hopefully @icing has some input here. It's unclear to me whether these sockets in the connection pool which are not being operated on will have CURL_WAIT_POLLIN / CURL_WAIT_POLLOUT /set and then are they maintained?, etc, basically I'd think we want to avoid any situation where we accidentally put the app into eating cpu because the socket just always stays at, say CURL_WAIT_POLLIN, because nothing is actually done on it when multi_perform is called but data is available to read (like SSL or HTTP/2 protocol messages arrives etc). I don't know enough about this area to make the determination.

@bagder
Copy link
Member

bagder commented Jan 19, 2025

The difference between *waitfds() and *fdset() is exactly those shutdown connections that should be monitored. Shutdown connections are connections held separately while being closed down "properly". This is crystal clear.

@bagder
Copy link
Member

bagder commented Jan 19, 2025

The reason this is not noticed more by users is probably because curl_multi_fdset() might not be used a lot these days, plus the fact that ignoring the shutdown connections for a while probably can go one without any noticeable impact for a while as well.

bagder added a commit that referenced this issue Jan 19, 2025
They were previously missing.

Follow-up from c9b95c0

Fixes #15156
Reported-by: Christopher Dannemiller
bagder added a commit that referenced this issue Jan 21, 2025
They were previously missing.

Follow-up from c9b95c0

Fixes #15156
Reported-by: Christopher Dannemiller
@bagder bagder closed this as completed in 7c2b325 Jan 22, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging a pull request may close this issue.

5 participants
@bagder @jay @chrisd-work and others