[pantsd] Ensure rust panics surface in output or daemon logs #4522

Merged
merged 8 commits into from Apr 28, 2017

Conversation

Projects
None yet
2 participants
@JieGhost
Contributor

JieGhost commented Apr 26, 2017

Problem

Currently, rust panic do not appear to output or daemon logs when running pants through daemons.

Solution

I implemented a custom rust panic handler and a set_panic_handler function in rust that's exposed to python. For pantsd, set_panic_handler will be called after pantsd has been forked. For daemon worker process and local pants run, set_panic_handler will be called inside GoalRunner.Factory._init_graph method.

I have also included a trigger_panic function that will cause panic in rust side for testing purpose. If rust panic happens in pantsd, I can see panic message in pantsd log. If rust panic happens in daemon worker process, I can see panic message in client's stderr.

Result

If rust panic happens in pantsd, users can see panic message in pantsd log. If rust panic happens in daemon worker process, users can see panic message in client's stderr.

@JieGhost JieGhost requested review from stuhood and kwlzn Apr 26, 2017

src/rust/engine/src/lib.rs
+#[no_mangle]
+pub extern fn trigger_panic() {
+ panic!("This is a manually triggered panic for testing purpose.");
+}

This comment has been minimized.

@kwlzn

kwlzn Apr 26, 2017

Member

would kill this since you're done with testing presumably, unless you're planning to call it in e.g. an integration test?

@kwlzn

kwlzn Apr 26, 2017

Member

would kill this since you're done with testing presumably, unless you're planning to call it in e.g. an integration test?

This comment has been minimized.

@JieGhost

JieGhost Apr 27, 2017

Contributor

removed.

@JieGhost

JieGhost Apr 27, 2017

Contributor

removed.

src/python/pants/bin/goal_runner.py
@@ -101,6 +101,10 @@ def _init_graph(self, use_engine, pants_ignore_patterns, build_ignore_patterns,
exclude_target_regexps=exclude_target_regexps,
subproject_roots=subproject_build_roots)
)
+
+ # Set panic handler on native library.
+ graph_helper.scheduler.set_panic_handler()

This comment has been minimized.

@kwlzn

kwlzn Apr 26, 2017

Member

feels a bit odd to me to dangle this off of the LocalScheduler instance since it's not scheduler specific?

GoalRunner already declares a Subsystem dependency on Native - so you could just dangle a set_panic_handler on Native and call this directly/earlier - potentially anytime after OptionsInitializer is called.

@kwlzn

kwlzn Apr 26, 2017

Member

feels a bit odd to me to dangle this off of the LocalScheduler instance since it's not scheduler specific?

GoalRunner already declares a Subsystem dependency on Native - so you could just dangle a set_panic_handler on Native and call this directly/earlier - potentially anytime after OptionsInitializer is called.

This comment has been minimized.

@JieGhost

JieGhost Apr 27, 2017

Contributor

I created native object in this place, since I can pass it directly to setup_legacy_graph method.

@JieGhost

JieGhost Apr 27, 2017

Contributor

I created native object in this place, since I can pass it directly to setup_legacy_graph method.

src/python/pants/pantsd/pants_daemon.py
@@ -195,4 +195,7 @@ def pre_fork(self):
def post_fork_child(self):
"""Post-fork() child callback for ProcessManager.daemonize()."""
+ for service in self._services:
+ service.post_fork()

This comment has been minimized.

@kwlzn

kwlzn Apr 26, 2017

Member

rather than plumbing a post_fork hook to all services and setting up the handler in the SchedulerService's post_fork, you might be better off just calling the panic handler installer directly - at exactly this location?

in PantsDaemonLauncher (a Subsystem that sets up pantsd) you can declare a dependency on the Native subsystem to get a handle to it - and then pass that directly into the PantsDaemon constructor for access to e.g. Native.global_instance().set_panic_handler().

@kwlzn

kwlzn Apr 26, 2017

Member

rather than plumbing a post_fork hook to all services and setting up the handler in the SchedulerService's post_fork, you might be better off just calling the panic handler installer directly - at exactly this location?

in PantsDaemonLauncher (a Subsystem that sets up pantsd) you can declare a dependency on the Native subsystem to get a handle to it - and then pass that directly into the PantsDaemon constructor for access to e.g. Native.global_instance().set_panic_handler().

This comment has been minimized.

@JieGhost

JieGhost Apr 27, 2017

Contributor

This sounds like a good idea. And by doing this, set panic handler once will cover both pantsd and daemon worker process, as they share 1 Native object (or forked from the shared object).

@JieGhost

JieGhost Apr 27, 2017

Contributor

This sounds like a good idea. And by doing this, set panic handler once will cover both pantsd and daemon worker process, as they share 1 Native object (or forked from the shared object).

src/rust/engine/src/lib.rs
+ externs::log(externs::LogLevel::Critical, &panic_str);
+
+ let mut panic_file_bug_str = "Oops! You should not see this. ".to_string();
+ panic_file_bug_str.push_str("Please file a bug at https://github.com/pantsbuild/pants/issues.");

This comment has been minimized.

@kwlzn

kwlzn Apr 26, 2017

Member

would kill the "oops you should not see this" part in favor of just the "Please file a bug at ..." string.

@kwlzn

kwlzn Apr 26, 2017

Member

would kill the "oops you should not see this" part in favor of just the "Please file a bug at ..." string.

This comment has been minimized.

@JieGhost

JieGhost Apr 27, 2017

Contributor

done.

@JieGhost

JieGhost Apr 27, 2017

Contributor

done.

@kwlzn

kwlzn approved these changes Apr 28, 2017

lgtm! two more quick thoughts.

src/python/pants/pantsd/pants_daemon.py
@@ -49,7 +49,7 @@ class StartupFailure(Exception): pass
class RuntimeFailure(Exception): pass
def __init__(self, build_root, work_dir, log_level, log_dir=None, services=None,
- metadata_base_dir=None, reset_func=None):
+ metadata_base_dir=None, reset_func=None, native=None):

This comment has been minimized.

@kwlzn

kwlzn Apr 28, 2017

Member

afaict, this should probably not default to None but instead be required.

@kwlzn

kwlzn Apr 28, 2017

Member

afaict, this should probably not default to None but instead be required.

This comment has been minimized.

@JieGhost

JieGhost Apr 28, 2017

Contributor

done.

@JieGhost

JieGhost Apr 28, 2017

Contributor

done.

src/python/pants/pantsd/pants_daemon.py
@@ -195,4 +196,7 @@ def pre_fork(self):
def post_fork_child(self):
"""Post-fork() child callback for ProcessManager.daemonize()."""
+ if self._native:
+ self._native.set_panic_handler()

This comment has been minimized.

@kwlzn

kwlzn Apr 28, 2017

Member

and then you could get rid of the if self._native check here.

@kwlzn

kwlzn Apr 28, 2017

Member

and then you could get rid of the if self._native check here.

This comment has been minimized.

@JieGhost

JieGhost Apr 28, 2017

Contributor

done.

@JieGhost

JieGhost Apr 28, 2017

Contributor

done.

src/rust/engine/src/lib.rs
+
+ if let Some(location) = panic_info.location() {
+ let panic_location_str = format!("{}:{}", location.file(), location.line());
+ panic_str.push_str(&panic_location_str);

This comment has been minimized.

@kwlzn

kwlzn Apr 28, 2017

Member

should probably move the inclusion of the trailing comma here to the later conditional push_str'd string?

if the panic_info.location() Option is None, some logs could look like: panic at '...', which might make them appear to be truncated.

@kwlzn

kwlzn Apr 28, 2017

Member

should probably move the inclusion of the trailing comma here to the later conditional push_str'd string?

if the panic_info.location() Option is None, some logs could look like: panic at '...', which might make them appear to be truncated.

This comment has been minimized.

@JieGhost

JieGhost Apr 28, 2017

Contributor

Good point! done.

@JieGhost

JieGhost Apr 28, 2017

Contributor

Good point! done.

@JieGhost JieGhost merged commit 6d0c0fb into pantsbuild:master Apr 28, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@JieGhost JieGhost deleted the JieGhost:yujieproject/rust_panic branch Apr 28, 2017

thesamet added a commit to thesamet/pants that referenced this pull request May 9, 2017

[pantsd] Ensure rust panics surface in output or daemon logs (#4522)
### Problem

Currently, rust panic do not appear to output or daemon logs when running pants through daemons.

### Solution

I implemented a custom rust panic handler and a set_panic_handler function in rust that's exposed to python. For pantsd, set_panic_handler will be called after pantsd has been forked. For daemon worker process and local pants run, set_panic_handler will be called inside GoalRunner.Factory._init_graph method.

I have also included a trigger_panic function that will cause panic in rust side for testing purpose. If rust panic happens in pantsd, I can see panic message in pantsd log. If rust panic happens in daemon worker process, I can see panic message in client's stderr.

### Result

 If rust panic happens in pantsd, users can see panic message in pantsd log. If rust panic happens in daemon worker process, users can see panic message in client's stderr.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment