Repository navigation
Keep request callbacks on the request, scope the bus dispatcher, and add an opt-in Redis pool - #24
Merged
Conversation
CoroutineApplication forwarded rebinding(), refresh() and terminating()
to the base application. AuthManager calls refresh('request', $guard,
'setRequest') for every guard, so each request left a callback that
holds the guard, request, session and user on the worker for good, and
the session guard got the base application's boot request instead of
the current one (remember-me and basic auth read an empty request).
Every terminating callback was kept too, and base->terminate() ran all
of them at the end of every later request: the Blade engine registers
one each time Octane re-resolves it, so a worker slowed down as it
aged, and a request's callbacks re-ran in other requests.
Inside a request these callbacks now live on the RequestScope:
rebinding a request-scoped abstract registers on the scope and returns
the scoped instance, and terminate() runs the boot-time callbacks and
then this request's. The bus dispatcher is scoped as well, because
dispatch()->afterResponse() and batches register on the dispatcher's
own container, which was the base application: an after-response job
ran again at the end of every later request on the worker.
Every request coroutine opened its own phpredis connections (connect, AUTH, SELECT) and closed them at the end, so each request paid those round trips and closed sockets piled up in TIME_WAIT until connects failed with "Cannot assign requested address". With octane.swoole.redis_pool.enabled, a clean socket goes back to a per-worker pool when the request is torn down and the next request borrows it in a new lease. It is reused only if it is still connected, outside MULTI or a pipeline, on the database and options it was created with, and ran no unfinished or state-changing command (including raw ones); otherwise it is closed. The returned lease loses the socket, so a reference kept past the request throws instead of sharing it. Borrowing never waits; idle sockets expire and are pinged after idle. Off by default; non-coroutine paths are unchanged. CI now runs a Redis service so the Redis tests run instead of skipping.
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.
Problems (found on modelslab-frontend-v2's production image under Swoole, with recycling off and a probe of base-container state)
refresh()/rebinding()went to the base app. AuthManager callsrefresh('request', $guard, 'setRequest')for every guard; the auth manager is request-scoped, so each request left one rebound callback (holding the guard → request → session → user) on the worker for good — and the session guard was handed the base app's boot request, not the current one, so anything it reads from "its" request (remember-me cookie, HTTP Basic credentials) read an empty request.api/sanctumguards (built withviaRequest) were fine.terminating()/terminate()went to the base app. Every request's callbacks were kept, andbase->terminate()ran all of them at the end of every later request. The Blade engine registers one each time it is resolved, and Octane forgets the blade/php engines after every request, so a worker kept and ran one more callback per request it had served — throughput fell as it aged, and any request callback re-ran in other requests.dispatch(...)->afterResponse()(andBus::batch) registered on the base terminating list (problem 2). Measured on the app: 1 job dispatched, then 40 other requests → the job ran 41 times.Cannot assign requested address(~370 req/s per container locally), and every request pays the round trips.Fix
CoroutineApplication::rebinding()/refresh(): inside a request, for an abstract the request scope holds (in practicerequest), the callback is stored on theRequestScopeand the scoped instance is returned;rebound()runs the request's callbacks. Outside a request: unchanged.CoroutineApplication::terminating()/terminate(): inside a request, callbacks go to theRequestScope;terminate()runs the boot-time (base) callbacks, then the request's. Outside a request: unchanged.Bus::fake()stays shared.octane.swoole.redis_pool.enabled, default off):RedisConnectionPool,PooledPhpRedisConnection,PooledPhpRedisConnector,CoroutineRedisResolver. Each borrow gets a new lease around a pooled socket; the worker returns it after the request is torn down (detached beforeflush(), released infinally, like the DB pool) — only if provably clean: connected, not in MULTI/pipeline, same db and phpredis options as at creation, and no unfinished or state-changing command, raw ones included (SUBSCRIBE, WATCH, SELECT, MULTI viaexecuteRaw, CLIENT, AUTH, HELLO, …). The returned lease loses the socket: a reference kept past the request throwsLogicExceptioninstead of sharing it. Never blocks;max_idle,max_idle_time(pruned from the bottom of the stack too),ping_after_idle,max_lifetime; pid guard; configs that cannot be serialized are not pooled. Non-coroutine paths and child coroutines (go()) unchanged.Verification
CoroutineRebindingTest(5),CoroutineTerminatingTest(6, incl. after-response job runs once),RedisConnectionPoolTest(17, real Redis). Reverting each fix fails its tests; each guard is mutation-tested (rebinding, terminating, dispatcher scoping, clean-state snapshot, stateful/raw-command flag, lease revoke, idle cap, bottom-of-stack expiry).executeRawbypassing the stateful check, sockets returned before teardown, LIFO idle never expiring, and the dispatcher's after-response path — all fixed above with tests.ABBA wrk (prod flags, recycling at 500; together with the app's own changes — Sentry view tracing off, pool on): render +22%, heavy render +87%, cache HIT +15%, JSON +16%. Sessions persist through pooled connections (0 new session keys over 20 requests); Vite preload headers 0/160 wrong; 0 errors in the log.
Known limits
go()inside a request) still reach the base app, and its Redis connections are not pooled (closed as before).LogicExceptiononce the request ends (before: it kept a socket that was closed).Need help on this PR? Tag
@codesmith-botwith what you need.