Thread-local stdio handling to support concurrent pantsd clients #11536

stuhood · 2021-02-09T00:30:29Z

Problem

In order to support concurrent pantsd clients, work spawned by each client (even into background threads) must interact with the relevant stdio file handles. And when a client disconnects, spawned work should be able to continue to log to the pantsd.log.

Solution

Extract the thread/task-local aspects of our logging crate into a stdio crate that provides:

a Console-aware logging destination
exclusive access to a Console while a UI or InteractiveProcess is running
Python-level sys.std* replacements

Result

No user-visible impact, but one of the largest remaining blockers for #7654 is removed.

### Problem #11536 moves from using POSIX-level replacement of the `stdio` file descriptors `{0, 1, 2}`, to replacing `sys.std*` with a thread-local implementation. Unfortunately, Python `subprocess`/`Popen` APIs hardcode `{0, 1, 2}` rather than actually inspecting `sys.std*.fileno()`, and so usages of those APIs that use `std*=None` (i.e. "inherit" mode), will inherit intentionally dead/closed file handles under `pantsd`. PEX uses inherit mode when spawning `pip` (in order to pass through `stderr` to the parent process), and this runs afoul of the above behavior. Pants has used PEX as a library for a very long time, but 95% of usecases have now migrated to using PEX as a binary, with wrappers exposed via the `@rule` API. The `PluginResolver` that Pants uses to resolve its own code is the last usage of PEX as a library. ### Solution In a series of commits, introduce a "bootstrap" `Scheduler` that is able to resolve plugins after an `OptionsBootstrapper` has been created, but before creating the `BuildConfiguration` (which contains plugin information). ### Result Although Pants has some stray references to PEX APIs, it no longer uses PEX as a library to resolve dependencies. #11536 and #7654 are unblocked. In future, if the options required to construct a minimal scheduler can be further pruned, the bootstrap `Scheduler` might also be able to be used to create the `OptionsBootstrapper`, which would allow for addressing #10360.

…ate, and implement stdio replacement in Rust. [ci skip-build-wheels]

stuhood · 2021-02-25T01:11:46Z

Hey folks: this is ready for review. Sorry for the huge commit.

tests/python/pants_test/init/test_logging.py

Eric-Arellano · 2021-02-25T01:24:04Z

Hey folks: this is ready for review. Sorry for the huge commit.

Any tips for it, like read this file first? Or things you want us to look out for?

gshuflin · 2021-02-25T02:30:34Z

src/rust/engine/Cargo.toml

@@ -147,3 +151,5 @@ env_logger = "0.5.4"
 prost = { git = "https://github.com/danburkert/prost", rev = "a1cccbcee343e2c444e1cd2738c7fba2599fc391" }
 prost-build = { git = "https://github.com/danburkert/prost",  rev = "a1cccbcee343e2c444e1cd2738c7fba2599fc391" }
 prost-types = { git = "https://github.com/danburkert/prost",  rev = "a1cccbcee343e2c444e1cd2738c7fba2599fc391" }
+# TODO: Stabilize before landing.
+console = { git = "https://github.com/stuhood/console", branch = "stuhood/term-target-for-arbitrary-handles" }


So right now we're relying on some changes you made in your own copy of this repo?

Yea, I'll fork them to pantsbuild/pants before landing.

src/rust/engine/logging/src/logger.rs

src/python/pants/core/goals/test_test.py

jsirois

LGTM.

It would be good to address the ~2 TODOs or file.

src/rust/engine/stdio/src/lib.rs

src/python/pants/bin/daemon_pants_runner.py

src/rust/engine/stdio/Cargo.toml

Eric-Arellano

Epic!

src/python/pants/backend/python/goals/pytest_runner_integration_test.py

src/python/pants/core/goals/repl_integration_test.py

src/python/pants/core/goals/run_test.py

src/python/pants/init/logging.py

src/rust/engine/stdio/src/lib.rs

gshuflin · 2021-03-01T20:40:46Z

src/rust/engine/stdio/src/lib.rs

+
+  ///
+  /// If stdout is backed by a real file, returns it as a RawFd. All usage of `RawFd` is unsafe,
+  /// but this method is additionally unsafe because the real file might have been closed by the


This comment is kind of confusing, since this method is not marked as a Rust unsafe function. IIRC the libc functions that manipulate RawFd are generally unsafe, but that's not the same thing as this function that generates RawFd being unsafe.

The unsafety here is technically just the same unsafety that is possible for any RawFd... this function will not directly panic: instead, the RawFd that you get might already be closed. So the comment is probably misleading, yea.

[ci skip-build-wheels]

stuhood · 2021-03-01T23:53:41Z

This was green aside from some flakes in Github Actions for macOS: opened #11617 for those.

### Problem A few references to the deprecated `.pants.d/pantsd/pantsd.log` location survived after #11536. ### Solution Fixup the references to point to the new location at `.pants.d/pants.log`. [ci skip-rust] [ci skip-build-wheels]

### Problem Since migrating to thread-local `stdio` in #11536, explicit stdio rendering methods on `Session`/`Scheduler` have been dead code. But additionally, our UI fallback could hypothetically have been interacting with `std::io::std*` on fds `1,2`, which would cause a crash. ### Solution Remove some dead (and some dangerous!) code around stdio in `Scheduler`/`Session`, and isolate stderr handling to the UI crate. ### Result Probably fixes #11626. [ci skip-build-wheels]

…`. (#11703) ### Problem The threadlocal implementation of `sys.stdin` added in #11536 did not expose enough methods to be used for `input`. ### Solution Implement the methods needed for `input`, and wrap for use buffered text consumption. ### Result Fixes #11398. [ci skip-build-wheels]

…t. (#11731) ### Problem `pantsd` has closed the file handles associated with `sys.std*` (fds "0, 1, 2") since its early inception, and until #11536 it allowed exactly one client at a time in order to redirect those precise fd numbers to the client socket (via nailgun). It's likely that: 1. before #11536, between the closing of stdio and its replacement when a client connected, any attempt to interact with `sys.std*` or Rust's stdio file handles would crash. 2. after #11536, interactions with `sys.std*` or Rust's stdio file handles at _any_ time would crash. ### Solution Rather than closing `sys.std*` in `pantsd`, replace them with valid file handles as a backstop against code that has-not/cannot be updated to avoid touching them (such as Rust panic handlers). In the case of `stdout`/`stderr`, redirect them to append to the `pants.log`. ### Result Given what was reported on tokio-rs/tokio#3499, it's likely that this fixes #11364 by replacing the "panicked while processing panic" message with an actual panic message rendered in the `pants.log` (for which we can open a new ticket). [ci skip-rust] [ci skip-build-wheels]

stuhood force-pushed the stuhood/threadlocal-io-destinations branch 3 times, most recently from 4219cdc to cc6eb14 Compare February 10, 2021 21:31

stuhood mentioned this pull request Feb 13, 2021

Re-enable concurrent runs for pantsd in v2 #7654

Open

stuhood force-pushed the stuhood/threadlocal-io-destinations branch 3 times, most recently from 53cf8e9 to cd21f44 Compare February 18, 2021 20:11

stuhood mentioned this pull request Feb 22, 2021

Resolve plugins via PEX rules in a bootstrap scheduler #11568

Merged

stuhood force-pushed the stuhood/threadlocal-io-destinations branch from cd21f44 to f07797f Compare February 24, 2021 01:00

Extract thread/task-local IO destinations to a lower level stdio cr…

d129bed

…ate, and implement stdio replacement in Rust. [ci skip-build-wheels]

stuhood force-pushed the stuhood/threadlocal-io-destinations branch from f07797f to d129bed Compare February 25, 2021 00:24

stuhood requested review from gshuflin, Eric-Arellano, jsirois and tdyas and removed request for gshuflin February 25, 2021 01:10

stuhood marked this pull request as ready for review February 25, 2021 01:11

gshuflin reviewed Feb 25, 2021

View reviewed changes

tests/python/pants_test/init/test_logging.py Show resolved Hide resolved

gshuflin reviewed Feb 25, 2021

View reviewed changes

src/rust/engine/logging/src/logger.rs Show resolved Hide resolved

tdyas approved these changes Feb 25, 2021

View reviewed changes

src/python/pants/core/goals/test_test.py Show resolved Hide resolved

jsirois approved these changes Feb 25, 2021

View reviewed changes

src/rust/engine/stdio/src/lib.rs Outdated Show resolved Hide resolved

stuhood commented Feb 25, 2021

View reviewed changes

src/python/pants/bin/daemon_pants_runner.py Outdated Show resolved Hide resolved

stuhood commented Feb 25, 2021

View reviewed changes

src/rust/engine/stdio/Cargo.toml Outdated Show resolved Hide resolved

Eric-Arellano approved these changes Feb 25, 2021

View reviewed changes

stuhood mentioned this pull request Mar 1, 2021

Log color is broken sometimes #11020

Closed

stuhood force-pushed the stuhood/threadlocal-io-destinations branch from cc7c446 to a5af6f4 Compare March 1, 2021 20:28

gshuflin reviewed Mar 1, 2021

View reviewed changes

Review feedback.

4078b32

[ci skip-build-wheels]

stuhood force-pushed the stuhood/threadlocal-io-destinations branch from a5af6f4 to 4078b32 Compare March 1, 2021 23:06

stuhood merged commit 87f2261 into pantsbuild:master Mar 1, 2021

stuhood deleted the stuhood/threadlocal-io-destinations branch March 1, 2021 23:53

This was referenced Mar 2, 2021

Add an input method to Console #11398

Closed

Wrap ./pants help based on actual terminal width #11378

Merged

Eric-Arellano mentioned this pull request Mar 3, 2021

Test results are not always showing up in UI #11626

Closed

This was referenced Mar 4, 2021

Workunit logging should be per-Session #11253

Closed

Fix a few references to the deprecated pants log location. #11636

Merged

stuhood mentioned this pull request Mar 6, 2021

Allow concurrent runs of pants on the same instance of pantsd #11639

Closed

Eric-Arellano mentioned this pull request Mar 8, 2021

Prepare 2.4.0.dev1 #11653

Merged

stuhood mentioned this pull request Mar 11, 2021

Avoid std::io::stdio usage in UI fallback #11671

Merged

stuhood mentioned this pull request Mar 16, 2021

Implement the IO methods required for input, and add Console.input. #11703

Merged

stuhood mentioned this pull request Mar 18, 2021

Redirect pantsd stdio to the pants.log rather than entirely closing it. #11731

Merged

asherf mentioned this pull request May 19, 2021

Lower the log level for scheduler init messages #12081

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Thread-local stdio handling to support concurrent pantsd clients #11536

Thread-local stdio handling to support concurrent pantsd clients #11536

stuhood commented Feb 9, 2021 •

edited

Loading

stuhood commented Feb 25, 2021

Eric-Arellano commented Feb 25, 2021

gshuflin Feb 25, 2021 •

edited

Loading

stuhood Mar 1, 2021

jsirois left a comment •

edited

Loading

Eric-Arellano left a comment

gshuflin Mar 1, 2021

stuhood Mar 1, 2021

stuhood commented Mar 1, 2021

Thread-local stdio handling to support concurrent pantsd clients #11536

Thread-local stdio handling to support concurrent pantsd clients #11536

Conversation

stuhood commented Feb 9, 2021 • edited Loading

Problem

Solution

Result

stuhood commented Feb 25, 2021

Eric-Arellano commented Feb 25, 2021

gshuflin Feb 25, 2021 • edited Loading

Choose a reason for hiding this comment

stuhood Mar 1, 2021

Choose a reason for hiding this comment

jsirois left a comment • edited Loading

Choose a reason for hiding this comment

Eric-Arellano left a comment

Choose a reason for hiding this comment

gshuflin Mar 1, 2021

Choose a reason for hiding this comment

stuhood Mar 1, 2021

Choose a reason for hiding this comment

stuhood commented Mar 1, 2021

stuhood commented Feb 9, 2021 •

edited

Loading

gshuflin Feb 25, 2021 •

edited

Loading

jsirois left a comment •

edited

Loading