]> git.ipfire.org Git - thirdparty/squid.git/commit
Bug 5352: Do not get stuck when RESPMOD is slower than read(2) (#1777)
authorAlex Rousskov <rousskov@measurement-factory.com>
Sat, 13 Apr 2024 08:15:00 +0000 (08:15 +0000)
committerSquid Anubis <squid-anubis@squid-cache.org>
Mon, 15 Apr 2024 22:37:33 +0000 (22:37 +0000)
commitcc8b26f9fa6a3dba5b490558652157b926e513f2
tree240daf2b8a8abbaca596ee0a28a7f41730228090
parent47126086215fae10ff16c5be390d876db3aedf15
Bug 5352: Do not get stuck when RESPMOD is slower than read(2) (#1777)

    ... RESPMOD BodyPipe buffer becomes full ...
    maybeMakeSpaceAvailable: will not read up to 0
    The AsyncCall Client::noteDelayAwareReadChance constructed

    ... RESPMOD consumes some buffered virgin body data ...
    entering BodyProducer::noteMoreBodySpaceAvailable
    leaving BodyProducer::noteMoreBodySpaceAvailable

    ... read_timeout seconds later ...
    http.cc(148) httpTimeout
    FwdState.cc(471) fail: ERR_READ_TIMEOUT "Gateway Timeout"

When RESPMOD does not empty its adaptation BodyPipe buffer fast enough,
readReply() may eventually fill that buffer and call delayRead(),
anticipating a noteDelayAwareReadChance() callback from Store or Server
delay pools. That callback never happens if Store and Server are not
getting any data -- they do not even start working until RESPMOD service
starts releasing adapted/echoed response back to Squid! Meanwhile, our
flags.do_next_read (cleared by readReply() caller) remains false.

When/if RESPMOD service eventually frees some BodyPipe buffer space,
triggering noteMoreBodySpaceAvailable() notification, nothing changes
because maybeReadVirginBody() quits when flags.do_next_read is false.

noteMoreBodySpaceAvailable() could not just make flags.do_next_read true
because that flag may be false for a variety of other/permanent reasons.
Instead, we replaced that one-size-fits-all flag with more specific
checks so that reading can resume if it is safe to resume it. This
change addresses a couple of flag-related XXXs.

The bug was introduced in 2023 commit 50c5af88. Prior that that change,
delayRead() was not called when RESPMOD BodyPipe buffer became full
because maybeMakeSpaceAvailable() returned false in that case, blocking
maybeReadVirginBody() from triggering readReply() via Comm::Read(). We
missed flags.do_next_read dependency and that Store-specific delayRead()
cannot be used to wait for adaptation buffer space to become available.

XXX: To reduce risks, this change duplicates a part of
calcBufferSpaceToReserve() logic. Removing that duplication requires
significant (and risky) refactoring of several related methods.
src/clients/Client.cc
src/clients/Client.h
src/clients/FtpClient.cc
src/http.cc
src/http.h
src/http/StateFlags.h