Add per-PTY dispatch-source I/O path as alternative to TaskNotifier select loop#587
Draft
chall37 wants to merge 1 commit into
Draft
Add per-PTY dispatch-source I/O path as alternative to TaskNotifier select loop#587chall37 wants to merge 1 commit into
chall37 wants to merge 1 commit into
Conversation
…loop Replace the shared TaskNotifier select() loop with per-PTY serial dispatch queues, each owning DispatchSource read/write/process sources. Gated behind the UsePerPTYDispatchSources advanced setting (default off). Key design: - PTYTaskIOHandler owns the ioQueue and all dispatch sources for one PTY - DISPATCH_SOURCE_TYPE_PROC monitors child exit to force-resume the read source for EOF delivery even when suspended for backpressure/pause - Backpressure integration: TokenExecutor tracks available slots via an atomic counter; when heavy backpressure is detected, wantsRead returns false and the read source suspends. A backpressureReleaseHandler wired in PTYSession re-evaluates read state when pressure drops - Backpressure-gated reads are restricted to per-PTY mode to avoid stalling the legacy TaskNotifier path which has no wake-up mechanism - Registration routing via PTYTask class methods so job managers do not need to know which I/O path is active - Coprocess sources are serialized on ioQueue to prevent data races with event handlers Includes comprehensive test coverage: lifecycle, state predicates, EOF detection, read/write round-trips, coprocess management, and concurrency regression tests.
chall37
force-pushed
the
feat/per-pty-dispatch-sources-pr
branch
from
d7f0a27 to
3360501
Compare
Owner
|
Thanks for taking the time to put this together! I'm trying to review it but I don't think I can do a good job on a PR of this size. This is critical infrastructure so it's better to be slow and methodical. Probably it would make the most sense to chat about it instead of trying to discuss it asynchronously in a PR. LMK what's a good time when you have an hour or so and we can talk through what's going on here and what would be the best way to proceed. I'm pretty slammed until Tuesday, so any time after that should be fine. |
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
2 participants
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.
Summary
an alternative to the shared TaskNotifier select() loop.
agnostic to the active I/O path.
PTYSession.taskDidRegister:.
data / broken-pipe.
path continues unchanged in this area).
Relationship To Fairness Scheduler
This PR was split out of #580 to stand alone per discussion. A few choices reflect that extraction:
none/light/moderate/heavy/blockedis introduced even though this PRcurrently gates only on
>= heavy. The extra levels are intended to support future scheduler policy/budgetdecisions.
wantsReadapplies backpressure gating only when anioHandlerexists.Legacy
TaskNotifiermode has no pressure-release wakeup, so gating there can stall reads. Future scheduler wiringcan unify this behavior across both paths.