smtp: give failures a hierarchy, and waits their own budgets - #131
Merged
Conversation
Two things a caller could not previously do. Tell a protocol failure from a mistake of their own. One flat Exception covered both, so an address that is not an address and a socket that died arrived as the same type -- seventeen of the thirty-two throw sites were that base. Runtime failures now extend Exception\SmtpException, which is one catch for anything the protocol threw; caller mistakes raise the SPL InvalidArgumentException and using a transport before connecting raises LogicException, so catching the package base cannot swallow a bug in the calling code. That is the line packages/storage draws. The move surfaced three misfilings. A failed fread on an attachment threw ConnectionException, which is a lie about what broke. A read length below one, and a transport used before connect, both threw ConnectionException when neither is a connection failure. And a server without STARTTLS is not a connection failure either -- it is the server saying what it can do, which CapabilityException now names, along with a message above the size it advertises and an address needing SMTPUTF8 it does not offer. Then timeouts. One value covered connecting, reading and writing, which forces a choice between noticing an outage quickly and letting a large message through -- the reply after the final dot can legitimately take minutes. Timeouts carries three, per operation, defaulting to ten seconds to connect and thirty either way after that. TimeoutException extends ConnectionException so indifference costs one catch and a retry decision costs two. Found on the way: Swoole::read compared errCode against SOCKET_ETIMEDOUT, a constant from ext-sockets, which this package does not require and Swoole does not provide -- a fatal on any host without it. Both transports now tell a timeout from a hang-up by the clock they set themselves. 185 unit and 28 end-to-end. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Contributor
Greptile SummaryThe PR introduces a structured SMTP exception hierarchy and separate connect, read, and write timeout budgets.
Confidence Score: 5/5The PR appears safe to merge because no blocking failure remains established on the current head. No blocking failure remains. Important Files Changed
Reviews (2): Last reviewed commit: "smtp: make the write budget real, and th..." | Re-trigger Greptile |
Three from review, all of them right. The coroutine transport took a write timeout and ignored it. recv() accepts a deadline per call but send() and enableSSL() do not -- they use whatever the client was configured with -- so a write budget was silently governed by the connect deadline instead. Swoole does support write_timeout and connect_timeout as settings, so both are now written to the client before the operation that needs them, cached so a large message does not reconfigure per chunk. A write or handshake that ran out of time still raised the generic connection failure, so a caller retrying on deadlines missed exactly the cases worth retrying. Both now classify: the stream transport asks stream_get_meta_data, which knows outright, and the coroutine one times its own call. And the positivity check let INF and NAN through -- INF passes every comparison and NAN fails every one, including the one meant to catch it, so a deadline of for ever or of nothing reached the socket. Rejected now, with the value formatted through var_export because coercing NAN to a string is itself a warning on PHP 8.5. The write budget is pinned by asking the Swoole client for its settings, since a deadline that never leaves PHP is invisible from the outside. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
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.
Follow-up to #130. Two things a caller could not previously do.
Telling a protocol failure from a mistake of their own
One flat
Utopia\SMTP\Exceptioncovered both, so an address that is not an address and a socket that died arrived as the same type. Seventeen of the thirty-two throw sites were that bare base.Runtime failures now extend
Exception\SmtpException:ConnectionExceptionTimeoutExceptionConnectionException.ProtocolExceptionAuthenticationExceptionTransactionExceptionReply.CapabilityExceptionMessageExceptionCaller mistakes stay outside that hierarchy and raise SPL types —
InvalidArgumentExceptionfor an address that is not an address or a header the message owns,LogicExceptionfor a transport used before it was connected. So catching the package base cannot swallow a bug in the calling code. That is the linepackages/storagedraws, and the reasoning is written into its base class, which is why I followed it rather than inventing one.packages/clientandpackages/natsboth namespace their exceptions the same way.TimeoutExceptionextendsConnectionExceptionrather than sitting beside it: a caller who does not care why the socket failed writes one catch, and a caller deciding whether to retry writes two.Three things the move surfaced
Reclassifying every throw site is what found these — each was a type that lied about what had happened.
freadon an attachment file threwConnectionException. Nothing was wrong with the connection.ConnectionException. Neither is a connection failure; they are a bad argument and a usage error.ConnectionExceptiontoo. Nothing failed — the server said what it can do, which is nowCapabilityException, alongside a message above the size it advertises and an address needingSMTPUTF8it does not offer.Waits with their own budgets
A single
float $timeoutcovered connecting, reading and writing. That forces a choice between noticing an outage quickly and letting a large message through: a host that is down deserves seconds, while the reply after the terminating dot can legitimately take minutes while the server scans the message — RFC 5321 section 4.5.3.2 asks for ten there.Defaults are ten seconds to connect and thirty to read or write, each applying per operation rather than per session, so a large message is bounded by its own size rather than by one deadline for the whole exchange. The specification wants six separate minimums; three is the useful part of that, and the deviation stays documented.
A timeout is now distinguishable from a hang-up, which it was not before — both were
ConnectionExceptionwith only the message differing.A latent fatal, found on the way
Swoole::read()comparederrCodeagainstSOCKET_ETIMEDOUT. That constant comes fromext-sockets, which this package does not require and Swoole does not provide — so on any host without it, the first slow read would have been a fatal error rather than an exception. Both transports now tell a timeout from a hang-up by the clock they set themselves, which is portable and needs no extension.Testing
ExceptionTestcovers the hierarchy as a promise rather than an implementation detail: what onecatchcovers, and what it deliberately does not. The caller-mistake test catchesSmtpExceptionfirst and fails there, so the guarantee breaks loudly if any validation ever starts raising a package type.TimeoutsTestproves a read that runs out of time is aTimeoutExceptionand a read on a closed socket is not, both against a local listener arranged to be silent — no unroutable address to depend on.185 unit and 28 end-to-end, up from 165 and 28.
One note on Rector: it removed a bare
(string) $message;statement as having no effect, which left anexpectExceptionunreachable and the test silently passing. The value is now consumed so it cannot be dropped again — worth knowing about, since a neutered test looks exactly like a green one.Breaking
Every exception moved namespace and the
timeoutparameter becametimeouts. Both are breaking, andsmtphas never been released, so there is no migration to write.Checks
bin/monorepo check smtp— Pint, PHPStan atlevel: max, Rectorbin/monorepo test smtp— 185 unit, then compose up and 28 end-to-endbin/monorepo validate,vale README.md docs packages— clean🤖 Generated with Claude Code