Skip to content

conncache: preserve connect-only connections at the host limit - #22902

Closed
ldm0 wants to merge 1 commit into
curl:masterfrom
ldm0:connect-only-host-limit
Closed

ldm0 wants to merge 1 commit into
curl:masterfrom
ldm0:connect-only-host-limit

Conversation

@ldm0

@ldm0 ldm0 commented Sep 10, 2026

Copy link
Copy Markdown
Contributor

Apply the global idle checks to the per-host limit so CONNECT_ONLY sockets remain available to the application.

The !conn->bits.close check is not required to fix the bug, but for consistency with cpool_get_oldest_idle().

@github-actions github-actions Bot added the tests label Sep 10, 2026
Apply global idle checks to the per-host limit so CONNECT_ONLY
sockets remain available to the application.
@ldm0
ldm0 force-pushed the connect-only-host-limit branch from 77dcb69 to f646962 Compare September 11, 2026 03:46

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.

🟢 Approval recommended

The focused logic change matches global-limit behavior and is covered by targeted regression tests.

Pull request overview

Preserves application-owned CONNECT_ONLY sockets when enforcing per-host connection limits.

Changes:

  • Excludes closing and CONNECT_ONLY connections from per-host eviction.
  • Adds regression tests for host and total connection limits.
  • Registers the new tests and shared test executable.
File summaries
File Description
lib/conncache.c Protects CONNECT_ONLY sockets from host-limit eviction.
tests/libtest/lib3231.c Implements both connection-limit tests.
tests/libtest/Makefile.inc Registers the test executable.
tests/data/test3231 Tests the per-host limit.
tests/data/test3232 Tests the total limit.
tests/data/Makefile.am Registers both test cases.
Review details
  • Files reviewed: 6/6 changed files
  • Comments generated: 0
  • Review effort level: Balanced

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

@bagder bagder closed this in 9e6673f Sep 11, 2026
@bagder

bagder commented Sep 11, 2026

Copy link
Copy Markdown
Member

Thanks!

bagder pushed a commit that referenced this pull request Sep 24, 2026
sws updates its connection-monitor state only when it parses a request.
Since 5232c16 it no longer clears the state after the first logged
disconnect, so the state a test sets with connection-monitor remains
after that test ends. If the next test that uses the same server closes
a connection before sws has parsed any request of that test, sws logs
[DISCONNECT] even if that test does not use connection-monitor.

Tests 3231 and 3232 do that: they close a CONNECT_ONLY connection, which
never sent anything, before the HTTP transfer sends its request. They
fail with an unexpected [DISCONNECT] in server.input whenever the state
is still enabled from a previous test, such as 3223 to 3226. With
parallel test runners this depends on which runner picks up the test,
so the failures look random. Most Debian package builds of 8.23.0-rc1
failed like this. To reproduce, run this in the tests directory:

  ./runtests.pl 3226 3231

When a connection closes while the test number is unset
(DOCNUMBER_NOTHING), read it from server.cmd, as sws already does for
requests without one. Then use the connection-monitor setting of that
test, or disable connection monitoring if there is no test number. A
test using connection-monitor now gets [DISCONNECT] logged for a
connection closed before its first request, regardless of the previous
test.

Ref: #22902
Follow-up to 5232c16 #22159
Closes #23072
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Development

Successfully merging this pull request may close these issues.

3 participants