Port the pantsd nailgun server to rust #9722

stuhood · 2020-05-08T01:00:48Z

Problem

The setup and teardown of each request made to the nailgun server in pantsd had become quite complicated over time... and consequently, slower than it needed to be.

Solution

Port pantsd's nailgun server to rust using the nails crate. Additionally, remove the Exiter class, which had accumulated excess responsibilities that can instead be handled by returning ExitCode values. Finally, fix a few broken windows including: double logging to pantsd, double help output, closed file errors on pantsd shutdown, and redundant setup codepaths.

Result

There is less code to maintain, and runs of ./pants --enable-pantsd help take ~1.7s, of which ~400ms are spent in the server. Fixes #9448, fixes #8243, fixes #8206, fixes #8127, fixes #7653, fixes #7613, fixes #7597.

stuhood · 2020-05-10T03:47:34Z

The commits are useful to review independently, although the last one is a doozy... sorry about that.

src/python/pants/option/global_options.py

stuhood · 2020-05-12T19:04:43Z

Ok, expecting this to pass CI now. I went a bit further and fully removed the exiter function... sys.excepthook doesn't need the hook to actually exit the process itself: it already handles exiting.

Eric-Arellano

Awesome. Great work.

src/python/pants/base/exception_sink.py

Eric-Arellano · 2020-05-12T18:03:28Z

src/python/pants/bin/daemon_pants_runner.py

-class _PantsRunFinishedWithFailureException(Exception):
-    """Allows representing a pants run that failed for legitimate reasons (e.g. the target failed to
-    compile).
+    def __init__(self, scheduler_service: SchedulerService):


Suggested change

def __init__(self, scheduler_service: SchedulerService):

def __init__(self, scheduler_service: SchedulerService) -> None:

In general we should be trying to add useful type annotations to old code when we modify it.

Eric-Arellano · 2020-05-12T18:03:51Z

src/python/pants/bin/daemon_pants_runner.py

-    Will be raised by the exiter passed to LocalPantsRunner.
-    """
+    @staticmethod
+    def _send_stderr(stderr_fd: int, msg: str):


Suggested change

def _send_stderr(stderr_fd: int, msg: str):

def _send_stderr(stderr_fd: int, msg: str) -> None:

Eric-Arellano · 2020-05-12T18:52:11Z

src/python/pants/bin/daemon_pants_runner.py

+        ), hermetic_environment_as(**env), argv_as((command,) + args):
+            # NB: Run implements exception handling, so only the most primitive errors will escape
+            # this function, where they will be logged to the pantsd.log by the server.
+            logger.info(f"handling request: `{' '.join(args)}`")


Why logger.info? That will show up by default.

It will only go to pantsd's log.

src/python/pants/bin/daemon_pants_runner.py

Eric-Arellano · 2020-05-12T18:59:19Z

src/python/pants/util/contextutil.py

+                termios.tcdrain(dst_fd)
+            else:
+                new_dst.flush()
+        except:  # noqa: E722


Suggested change

except: # noqa: E722

except BaseException:

Eric-Arellano · 2020-05-12T19:02:30Z

src/python/pants/util/contextutil.py

@@ -84,6 +85,17 @@ def hermetic_environment_as(**kwargs: Optional[str]) -> Iterator[None]:
        _restore_env(old_environment)


+@contextmanager
+def argv_as(args: Tuple[str, ...]) -> Iterator[None]:


This should probably have a unit test.

Eric-Arellano · 2020-05-12T19:07:34Z

src/python/pants/engine/internals/scheduler.py

+        """
+        return cast(int, self._native.nailgun_server_await_bound(self._scheduler, nailgun_server))
+
+    def new_nailgun_server(self, port_requested: int, runner: RawFdRunner) -> Any:


Is this Any because it truly could be Any, or because you don't know the more precise type? If the latter, drop the return type.

We don't know the precise type because it is coming out of CFFI. Will drop.

Eric-Arellano · 2020-05-12T19:08:00Z

src/python/pants/engine/internals/native.py

+        result = self.lib.nailgun_server_await_bound(scheduler, nailgun_server)
+        return cast(int, self.context.raise_or_return(result))
+
+    def new_nailgun_server(self, scheduler, port: int, runner: RawFdRunner) -> Any:


See comment in scheduler.py on the Any return type.

gshuflin · 2020-05-12T22:05:54Z

Excited to see a lot of the cruft related to Exiters go away :)

…restart. # Delete this line to force CI to run Clippy and the Rust tests. [ci skip-rust-tests] # Delete this line to force CI to run the JVM tests. [ci skip-jvm-tests]

[ci skip-jvm-tests]

…process (and only called for a signal in pantsd). [ci skip-jvm-tests]

…hook` doesn't need us to actually exit. [ci skip-jvm-tests]

# Delete this line to force CI to run the JVM tests. [ci skip-jvm-tests]

stuhood force-pushed the stuhood/pantsd-server-in-rust branch 2 times, most recently from 3e7ad64 to 48dc7bf Compare May 10, 2020 03:18

stuhood requested review from Eric-Arellano, gshuflin and jsirois May 10, 2020 03:35

stuhood marked this pull request as ready for review May 10, 2020 03:35

stuhood force-pushed the stuhood/pantsd-server-in-rust branch from 48dc7bf to c69395e Compare May 10, 2020 03:46

stuhood mentioned this pull request May 10, 2020

Enable pantsd by default #4438

Closed

Eric-Arellano reviewed May 10, 2020

View reviewed changes

src/python/pants/option/global_options.py Outdated Show resolved Hide resolved

stuhood force-pushed the stuhood/pantsd-server-in-rust branch 2 times, most recently from 0970711 to 89b159e Compare May 12, 2020 04:07

Eric-Arellano approved these changes May 12, 2020

View reviewed changes

gshuflin approved these changes May 12, 2020

View reviewed changes

Stu Hood added 6 commits May 12, 2020 20:27

Do not fully initialize pantsd in the client during either launch or …

7dd3967

…restart. # Delete this line to force CI to run Clippy and the Rust tests. [ci skip-rust-tests] # Delete this line to force CI to run the JVM tests. [ci skip-jvm-tests]

Add rust nailgun server with a callback to python.

bab9b94

[ci skip-jvm-tests]

Remove dead nailgun server code.

d697eda

[ci skip-jvm-tests]

Remove nested Exiters in favor of a single exit function set per-…

3d3dca6

…process (and only called for a signal in pantsd). [ci skip-jvm-tests]

Go a bit further and completely remove the exit function: `sys.except…

1015be4

…hook` doesn't need us to actually exit. [ci skip-jvm-tests]

Review feedback.

2e3b8fc

# Delete this line to force CI to run the JVM tests. [ci skip-jvm-tests]

stuhood force-pushed the stuhood/pantsd-server-in-rust branch from 7dde6d3 to 2e3b8fc Compare May 13, 2020 03:39

stuhood merged commit 9506fbf into pantsbuild:master May 13, 2020

stuhood deleted the stuhood/pantsd-server-in-rust branch May 13, 2020 04:55

This was referenced May 17, 2020

Install signal handler, to shutdown daemon on Ctrl-C #7668

Closed

Fix Metaspace OOM when using local hermetic nailgun execution strategy. #8750

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Port the pantsd nailgun server to rust #9722

Port the pantsd nailgun server to rust #9722

stuhood commented May 8, 2020 •

edited

Loading

stuhood commented May 10, 2020

stuhood commented May 12, 2020

Eric-Arellano left a comment

Eric-Arellano May 12, 2020

gshuflin May 12, 2020

Eric-Arellano May 12, 2020

Eric-Arellano May 12, 2020

stuhood May 12, 2020

Eric-Arellano May 12, 2020

Eric-Arellano May 12, 2020

Eric-Arellano May 12, 2020

stuhood May 12, 2020 •

edited

Loading

Eric-Arellano May 12, 2020

gshuflin commented May 12, 2020

	def __init__(self, scheduler_service: SchedulerService):
	def __init__(self, scheduler_service: SchedulerService) -> None:

	def _send_stderr(stderr_fd: int, msg: str):
	def _send_stderr(stderr_fd: int, msg: str) -> None:

Port the pantsd nailgun server to rust #9722

Port the pantsd nailgun server to rust #9722

Conversation

stuhood commented May 8, 2020 • edited Loading

Problem

Solution

Result

stuhood commented May 10, 2020

stuhood commented May 12, 2020

Eric-Arellano left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stuhood May 12, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gshuflin commented May 12, 2020

stuhood commented May 8, 2020 •

edited

Loading

stuhood May 12, 2020 •

edited

Loading