Skip to content

url match, no force reuse and candidates - #22857

Closed
icing wants to merge 5 commits into
curl:masterfrom
icing:url-match-the-umpteeth
Closed

icing wants to merge 5 commits into
curl:masterfrom
icing:url-match-the-umpteeth

Conversation

@icing

@icing icing commented Sep 7, 2026 •

Copy link
Copy Markdown
Contributor

Changes to connection matching for a transfer:

  • When a transfer previously used a connection and that still exists in the connection bundle to search and that still matches, use exactly that one. This eliminates the force_reuse bit in match making.
  • Always apply all match conditions before selecting a match.
  • NTML/Negotiate: check for creds match when conn->creds_origin is present. This should be the safe indicator that authenticaion is happening for the connection (as compared to requests) and credentials then need to match for it.

@icing
icing force-pushed the url-match-the-umpteeth branch from 2bbc4d0 to daac3a9 Compare September 7, 2026 12:07
@curl curl deleted a comment from testclutch Sep 7, 2026
@icing
icing marked this pull request as ready for review September 8, 2026 15:53
@icing
icing requested a review from bagder September 8, 2026 16:51
@bagder
bagder requested a balanced review from Copilot September 8, 2026 21:22

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

Connection validation can be bypassed, and stale wait_pipe state can incorrectly make transfers pending.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Pull request overview

Updates connection matching to prioritize a transfer’s previous connection and revise NTLM/Negotiate credential matching.

Changes:

  • Prioritizes lastconnect_id during pool searches.
  • Removes force_reuse and applies all matching conditions.
  • Uses credential-origin binding for authentication matching.

Review findings:

  • lib/url.c:1033: Previous-connection reuse bypasses lifetime, idle, and dead-connection checks.
  • lib/url.c:1025: wait_pipe can remain set after later matching checks reject a candidate.
  • lib/url.c:1027: Replace “it's” with the possessive “its.”
File summaries
File Description
lib/url.c Revises connection selection and authentication matching.
lib/conncache.c Searches the previous connection before other candidates.
Review details

Suppressed comments (1)

lib/url.c:1027

  • Use the possessive its rather than it's (“it is”).
    /* The connection fits, but it's multiplex state has not been determined
  • Files reviewed: 2/2 changed files
  • Comments generated: 2
  • Review effort level: Balanced

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread lib/url.c Outdated
Comment thread lib/url.c Outdated
Changes to connection matching for a transfer:

- When a transfer previously used a connection and that still
  exists in the connection bundle to search *and* that still
  matches, use exactly that one.
  This eliminates the `force_reuse` bit in match making.
- Always apply all match conditions before selecting a match
  or candidate.
- Store a connection for transfers that want NTLM/Negotiate as
  candidate in the matcher, to be used when no more, suitable
  connection is found.
- NTML/Negotiate: check for creds match when `conn->creds_origin`
  is present. This should be the safe indicator that authenticaion
  is happening for the connection (as compared to requests) and
  credentials then need to match for it.
- set pipe_wait in matcher only after all conditions have been met
- Curl_cpool_find() do not check a connection again when it was
  already tried on `data->state.lastconnect_id`.
@icing
icing force-pushed the url-match-the-umpteeth branch from dfcf0b5 to 1b1bef0 Compare September 9, 2026 12:52
@icing
icing requested a review from bagder September 9, 2026 13:21
@bagder bagder closed this in 7c11f1a Sep 9, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

3 participants