Skip to content

Fix SocketHandler firing writingTimeout instantly instead of waiting - #2071

Open
wakqasahmed wants to merge 2 commits into
Seldaek:mainfrom
wakqasahmed:fix/socket-handler-write-timeout-init
Open

wakqasahmed wants to merge 2 commits into
Seldaek:mainfrom
wakqasahmed:fix/socket-handler-write-timeout-init

Conversation

@wakqasahmed

Copy link
Copy Markdown
Contributor

SocketHandler::writeToSocket() resets $lastSentBytes to 0 at the top of every call, but never touches $lastWritingAt, which starts out as null. If the very first fwrite() in a call makes zero progress, writingIsTimedOut() sees $sent === $lastSentBytes (both 0) and takes the stalled branch, then compares microtime(true) against (float) $this->lastWritingAt. Since that's still null, it casts to 0.0, so the elapsed-time check is always true on the very first stall — the socket gets closed and a timeout exception is thrown in a fraction of a millisecond, instead of after the configured writingTimeout seconds.

There's already a test (testAvoidInfiniteLoopWhenNoDataIsWrittenForAWritingTimeoutSeconds) that mocks fwrite to always return 0, but it only asserts that a RuntimeException is eventually thrown, not when — so it's been passing the whole time despite the timeout never actually being honored.

Fix is a one-line initialization: set $lastWritingAt = microtime(true) alongside the existing $lastSentBytes reset, so the first stall check has a real baseline to measure from.

Added a new test that mocks the same always-stalled fwrite/streamGetMetadata scenario but asserts on timing: it sets a short writingTimeout, times how long writeToSocket takes to throw, and checks that the elapsed time is actually close to the configured timeout rather than near-zero. I verified this test fails against the current code (throws in ~0.0007s against a 0.3s timeout) and passes with the fix applied.

Ran the full SocketHandlerTest suite locally (PHP 8.3): 21 passed, 0 failed.

lastWritingAt was left null when a fresh write began, so if the very
first fwrite() made no progress, the stall check compared microtime()
against (float) null (0.0) and always treated the timeout as already
elapsed. Reset lastWritingAt alongside lastSentBytes at the top of
writeToSocket() so the first stall check has a real baseline.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant