Split shutdown handling from connection pool #16508
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
icing
force-pushed
the
cpool-shutdowns
branch
2 times, most recently
from
b59ed08 to
aa3ede6
Compare
|
@vszakats the winbuild seems to have overflown the max command line length on winbuilds. I added one source file. Anything I can do? |
Oh, that's a classic :/ I barely touched winbuild, so not sure what workaround is available. The usual escape route is to move the lists to a |
bagder
added a commit
that referenced
this pull request
Feb 28, 2025
strip: https://learn.microsoft.com/en-us/cpp/build/reference/nmake-function-strip?view=msvc-170 Trim off spaces when getting the list of objects in an attempt to avoid reaching "too long line". Ref: #16508
This comment was marked as resolved.
This comment was marked as resolved.
vszakats
added a commit
that referenced
this pull request
Mar 1, 2025
Keep the `@for %%i in [...]` lines within limits by stripping whitespace
from the input `.c` source lists read from `Makefile.inc`. To avoid this
error after adding a new `.c` source:
```
configuration name: libcurl-vc14-x64-release-dll-ssl-dll-ipv6-sspi
NMAKE : fatal error U1095: expanded command line 'for %i in (altsvc.obj amigaos.obj
asyn-ares.obj asyn-thread.obj base64.obj bufq.obj
bufref.obj cf-h1-proxy.obj cf-h2-proxy.obj cf-haproxy.obj [...]
vssh/wolfssh.obj) do @echo ..\builds\libcurl-vc14-x64-release-dll-ssl-dll-ipv6-sspi-obj-lib/%i \
' too long
Stop.
Command exited with code 2
```
Ref: https://ci.appveyor.com/project/curlorg/curl/builds/51605338/job/dqg6qtebtscb279g#L44
Reported-by: Stefan Eissing
Bug: #16508 (comment)
Fixes #16521
Closes #16528
Further testing with timeouts in event based processing
revealed that our current shutdown handling in the connection
pool was not clear enough. Graceful shutdowns can only happen
inside a multi handle and it was confusing to track in the code
which situation actually applies. It seems better to split
the shutdown handling off and have that code always be part
of a multi handle.
Add `cshutdn.[ch]` with its own struct to maintain connections
being shut down. A `cshutdn` always belongs to a multi handle
and uses that for socket/timeout monitoring.
The `cpool`, which can be part of a multi or share, either
passes connections to a `cshutdn` or terminates them with a
one-time, best effort.
Add an `admin` easy handle to each multi and share. This is
used to perform all maintenance operations where no "real"
easy handle is available. This solves the problem
that the multi admin handle requires some additional
initialisation (e.g. timeout list).
The share needs its admin handle as it is often cleaned up
when no other transfer or multi handle exists any more. But
we need a `data` in almost every call.
Fix file:// handling of errors when adding a new connection
to the pool.
Changes in `curl` itself:
- for parallel transfers, do not set a connection pool in the share,
rely on the multi's connection pool instead.
While not a requirement for the new `cshutdn` to work, this is
a) helpful in testing to trigger graceful shutdowns
b) a broader code coverage of libcurl via the curl tool
- on test_event with uv, cleanup the multi handle before returning
from parallel_event(). The uv struct is on the stack, cleanup
of the multi later will crash when it tries to register sockets.
This is a "eat your own dogfoof" related fix.
Try setting a shorter server_response_timeout as a stop gap.
- set the default shutdown timeout of 2 seconds for the internal admin handle when using that to disconnect the protocol - pingpong: add checking the over all timeout of the transfer during disconnect - pingpong: on poll timeout in blocking wait, error with CURLE_OPERATION_TIMEDOUT when disconnecting - multi: readd the timeout/signal "inheritance" of added transfers to the admin handle - as it was before.
bagder
approved these changes
Mar 2, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Further testing with timeouts in event based processing revealed that our current shutdown handling in the connection pool was not clear enough. Graceful shutdowns can only happen inside a multi handle and it was confusing to track in the code which situation actually applies. It seems better to split the shutdown handling off and have that code always be part of a multi handle.
Add
cshutdn.[ch]with its own struct to maintain connections being shut down. Acshutdnalways belongs to a multi handle and uses that for socket/timeout monitoring.The
cpool, which can be part of a multi or share, either passes connections to acshutdnor terminates them with a one-time, best effort.Add an
admineasy handle to each multi and share. This is used to perform all maintenance operations where no "real" easy handle is available.cpoolandcshutdndo no longer maintain their own internal handle. This solves the problem that the multi admin handle requires some additional initialisation (e.g. timeout list).The share needs its admin handle as it is often cleaned up when no other transfer or multi handle exists any more. But we need a
datain almost every call.Changes in
curlitself:cshutdnto work, this is a) helpful in testing to trigger graceful shutdowns b) a broader code coverage of libcurl via the curl tool