Fix Folder::idle() throwing after silent periods and busy-looping on … - #624
Open
mathe42 wants to merge 1 commit into
Open
Fix Folder::idle() throwing after silent periods and busy-looping on …#624mathe42 wants to merge 1 commit into
mathe42 wants to merge 1 commit into
Conversation
…closed connections
This file contains hidden or 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
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.
Fixes #558.
The bugs
Two defects in the catch block of
Folder::idle()combine to make long-running IDLE unreliable:1. The
strposcomparison is always true.strpos()returnsfalsewhen the needle is not found, and in PHPfalse >= 0evaluates totrue(the intended check was!== false). So the first guardmatches every
RuntimeExceptionand the decision rests entirely onconnected().2.
connected()cannot be used as a liveness probe here.ImapProtocol::connected()sends aNOOPviarequestAndResponse()— but the$idle_clientsession is in IDLE state, where the only valid clientinput is
DONE(RFC 2177). Depending on the server, the probe gets aBAD Expected DONE(→ImapServerErrorException) or no response for its tag at all (→ another read timeout), so it reportsfalseeventhough the connection is perfectly alive.
Resulting behaviour
$timeoutseconds without any server event, the stream read times out,nextLine()throwsempty response, the NOOP probe fails (see above), the guard falls through, and since themessage does not contain
connection closed,idle()throwsempty response— instead of simply continuing to wait.idle()therefore cannot outlive a single silent timeout window. This matches manylong-standing "empty response" reports.
imap.ionos.de:993):* BYE timeoutafter 1811s, then EOF (stream metatimed_out=true eof=true). Upstream code throws here as well instead of re-establishing the session.connected()reportstrueon a dead stream — e.g. the baseProtocol::connected()implementation, which is justreturn (bool)$this->stream;and staystrueafter thepeer closes the socket — the always-true
strposguard turns the loop intowhile (true) { continue; }: at EOF, reads return instantly, so the loop spins at 100% CPU forever. (Verified: with the NOOP probeabsent, the IONOS disconnect above sends the loop into a permanent full-core spin.)
The fix
Drop the in-IDLE liveness probe entirely. After a failed read,
stream_get_meta_data()(already exposed viaProtocol::meta()) reliably tells the two cases apart — the meta data is inconclusive onlybefore a read attempt, not after one (this sidesteps the stale-resource detection problem described in the comment further down in
idle()):timed_outand noteof→ plain read timeout, the connection is alive: keep waiting for the next event. No NOOP is sent, so the IDLE session stays intact.eof(or anything else) → the connection is gone: reset the idle client, reconnect, reopen the folder and re-issueIDLE, then continue waiting.This also removes a fall-through: previously a
connection closedexception dropped out of the catch block and re-processed the stale$linefrom the previous loop iteration.With this change
idle()survives quiet periods and server-side disconnects indefinitely (verified against the same IONOS server: 15 silent timeout windows handled cleanly, reconnect after* BYEworks, CPUstays at ~0% while idling).
Relation to #613
#613 addresses the NOOP-during-IDLE protocol violation from #558 by making
connected()skip the probe while idling. That fixes the protocol violation, but on its own it arms the busy-loop path describedabove: with the probe skipped,
connected()reportstrueon a dead stream, and the always-truestrposguard then turns a server-side disconnect into a permanent 100% CPU spin (this is exactly theconfiguration we measured the busy loop with). This PR removes the probe from the decision entirely, so both the silent-period case and the disconnect case are handled without relying on
connected().disclaimer
I used AI to debug and fix this issue. I verified the result and the bug and understand the code change.