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

multi: unlink handles in pending or msgsent states #10762

Closed
wants to merge 2 commits into from

Conversation

bagder
Copy link
Member

@bagder bagder commented Mar 14, 2023

No description provided.

@bagder
Copy link
Member Author

bagder commented Mar 14, 2023

I'll also make a test to move completed handles over to a new list so that they can be skipped in the main loop as well.

@bagder bagder changed the title multi: unlink handes in pending state multi: unlink handles in pending or msgsent states Mar 14, 2023
@bagder
Copy link
Member Author

bagder commented Mar 14, 2023

The pytest failures are very curious. I need to dig into those...

@bagder
Copy link
Member Author

bagder commented Mar 14, 2023

@icing I could use a little hand-holding to learn the best way to debug these pytest failures.

@icing
Copy link
Contributor

icing commented Mar 14, 2023

@icing I could use a little hand-holding to learn the best way to debug these pytest failures.

I'll checkout this branch and build locally in a few minutes.

@icing
Copy link
Contributor

icing commented Mar 14, 2023

I see problems locally, from runs stalling for a second to busy loops. The CI failures look like busy loops where the started curl simply does not return.

I have not pinned down the cause. But the order in which transfers are started is no longer the same as before. This stems from the fact that PENDING transfer are no longer processed at all. Simple example with 3 transfers:

start: 1-CONNECT, 2-CONNECT, 3-CONNECT
process 1: 1-CONNECTING, 2-CONNECT, 3-CONNECT
process 2: 1-CONNECTING, 2-PENDING, 3-CONNECT
2 is moved to pending and removed from easy list
           1-CONNECTING, 3-CONNECT
process 1: 1-PROTOCONNECT, 3-CONNECT
multi changed and pending is inspected
           1-PROTOCONNECT, 3-CONNECT, 2-CONNECT
process 1: 1-DO, 3-CONNECT, 2-CONNECT
process 1: 1-DID, 3-CONNECT, 2-CONNECT
process 1: 1-PERFORMING, 3-CONNECT, 2-CONNECT
process 1: 1-DONE, 3-CONNECT, 2-CONNECT
process 3: goes through all states
*** stalled for 1 second **
process 2: goes through all states

I think the problem is that you modify mult->easyp while iterating over it.

if(data) {
  do {
    run_single(data);   /* this may modify the list */
    data = data->next;
  } while(data);
}   

@bagder
Copy link
Member Author

bagder commented Mar 14, 2023

I think the problem is that you modify mult->easyp while iterating over it.

Ah yes indeed.

As they are not driving transfers or any socket activity, the main loop
does not need to iterate over these handles. A performance improvement.

They are instead only held in their own separate lists.

Ref: #10743
@bagder
Copy link
Member Author

bagder commented Mar 15, 2023

@icing thanks for that hint, it seems that was the bug that held this back.

@icing
Copy link
Contributor

icing commented Mar 15, 2023

@icing thanks for that hint, it seems that was the bug that held this back.

Yep, works now locally here as well.

@bagder bagder closed this in f6d6f3c Mar 15, 2023
@bagder bagder deleted the bagder/easy-links branch March 15, 2023 09:39
bch pushed a commit to bch/curl that referenced this pull request Jul 19, 2023
As they are not driving transfers or any socket activity, the main loop
does not need to iterate over these handles. A performance improvement.

They are instead only held in their own separate lists.

Assisted-by: Stefan Eissing
Ref: curl#10743
Closes curl#10762
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

None yet

2 participants