Clean up SubprocessInterface, JobManagerInterface and ChildJobInterface. |
|||
Issue descriptionFor ChromeOS session_manager. It seems like, the responsibility of each class is; - SubprocessInterface: Wrapping syscalls to talk with a subprocess. - JobManagerInterface: Interface of a manager class handling multiple subprocesses at once. - ChildJobInterface: Interface of a manager class handling a single subprocess at once. However, actual use case does not reflect the concept. Specifically; 1) JobManaerInterface is inherited by ContainerManagerInterface (= ARC), KeyGeneator, VpdProcess. All of them managed one process at once. So, it's better to migrate it into somehow ChildJobInterface. 2) ChildJobInterface is inherited by a browser and GeneratorJob. Each can fork/exec at most one-process, and the several ChildJobInterface API almost just redirects to Subprocess. So stepping back, it's nice to check if we'd like to share APIs between those Jobs. If it is not beneficial, removing the interface and define method for each concrete class may simplify the code. For better API design, I need to dive into the code deeper. I'll keep you updated.
,
Apr 2 2018
Looked into a bit more. JobManagerInterface is used to observe subprocess-exit signal. So, IsManagedJob and HandleExit are nice to be kept, and shared. RequestJobExit/EnsureJobExit are not necessary. I think it is ok just to drop them from the interface. Maybe rename it to JobExitObserver or something clears its responsibility. Note: this helps me to fix a bug I'm looking at, which needs to propagate exit reason to the caller, but the needed exit reason looks much different from the one for SessionManagerService. SubprocessInterface is to define the utility Subprocess and its Mock, so we can just leave it as is. ChildJobInterface is inherited by BrowserJob and GeneratorJob. However, there's nothing to be shared at the moment. We can remove the interface, and simplify the APIs in both classes by removing some glue code. WDYT?
,
Apr 2 2018
> RequestJobExit/EnsureJobExit are not necessary. > I think it is ok just to drop them from the interface. I see both of these methods called in various places for various implementations in session_manager_service.cc. Is it the case that outside of the ARC container, we only call these during shutdown and could instead just ensure that the subprocess exits during destruction? Or do you mean that we could just remove these methods from the interface and just make them be public non-virtual methods of the derived classes instead? In either case, I'm fine with that. I'd much prefer that we only use interfaces when it's actually necessary (because we actually need to do the same thing to multiple implementations somewhere, or because we want to inject stubs for e.g. unit testing). I think that the current code probably leans to far in the direction of introducing interfaces just because classes look somewhat similar.
,
Apr 3 2018
Thank you for feedback. > Is it the case that outside of the ARC container, we only call these during shutdown and could instead just ensure that the subprocess exits during destruction? Or do you mean that we could just remove these methods from the interface and just make them be public non-virtual methods of the derived classes instead? In either case, I'm fine with that. I meant latter. We're currently request to exit for all sub jobs, then wait them. If we move it to dtor for each job manager, then we need request to exit and then wait for each, i.e., we cannot request to exit in parallel, that could have some performance regression concern, IMO. > I'd much prefer that we only use interfaces when it's actually necessary Totally agreed. Let's give it a try to move that direction.
,
Apr 5 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromiumos/platform2/+/c4fce3b1e18f2118a497b057616f1c84c1427c68 commit c4fce3b1e18f2118a497b057616f1c84c1427c68 Author: Hidehiko Abe <hidehiko@chromium.org> Date: Thu Apr 05 03:55:29 2018 login: Remove {Request,Ensure}JobExit from JobManagerInterface. Those are not used in polymorphic way. Remove them from the interface, and instead define them in each concrete classes. This allows to customize these functions in later CL. BUG=chromium:827933 TEST=Ran on DUT. cros_run_unit_tests. Change-Id: I58e2d0028b60b0421af61f4521660f5f51bce385 Reviewed-on: https://chromium-review.googlesource.com/993632 Commit-Ready: Hidehiko Abe <hidehiko@chromium.org> Tested-by: Hidehiko Abe <hidehiko@chromium.org> Reviewed-by: Dan Erat <derat@chromium.org> [modify] https://crrev.com/c4fce3b1e18f2118a497b057616f1c84c1427c68/login_manager/container_manager_interface.h [modify] https://crrev.com/c4fce3b1e18f2118a497b057616f1c84c1427c68/login_manager/key_generator.cc [modify] https://crrev.com/c4fce3b1e18f2118a497b057616f1c84c1427c68/login_manager/session_manager_service.cc [modify] https://crrev.com/c4fce3b1e18f2118a497b057616f1c84c1427c68/login_manager/session_manager_service.h [modify] https://crrev.com/c4fce3b1e18f2118a497b057616f1c84c1427c68/login_manager/job_manager.h [modify] https://crrev.com/c4fce3b1e18f2118a497b057616f1c84c1427c68/login_manager/child_exit_handler_unittest.cc [modify] https://crrev.com/c4fce3b1e18f2118a497b057616f1c84c1427c68/login_manager/key_generator.h [modify] https://crrev.com/c4fce3b1e18f2118a497b057616f1c84c1427c68/login_manager/vpd_process_impl.cc [modify] https://crrev.com/c4fce3b1e18f2118a497b057616f1c84c1427c68/login_manager/vpd_process_impl.h
,
May 10 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromiumos/platform2/+/ee122ea83f2971f7dafef6fa35e6cf15df9f08dc commit ee122ea83f2971f7dafef6fa35e6cf15df9f08dc Author: Hidehiko Abe <hidehiko@chromium.org> Date: Thu May 10 22:10:11 2018 login: Clean up ChildExitHandler part 1. - Rename ChildExitHandler to ChildExitDispatcher. - Get rid of Init/Reset, instead use ctor/dtor with unique_ptr for simpler RAII. - Use C++11/14 new features. - Use D?CHECK_* family for equality check. BUG=chromium:827933 TEST=cros_run_unit_tests --package chromeos-login. Change-Id: I6d5ed16851c061741309e82818d85311595f77ba Reviewed-on: https://chromium-review.googlesource.com/1053038 Commit-Ready: Hidehiko Abe <hidehiko@chromium.org> Tested-by: Hidehiko Abe <hidehiko@chromium.org> Reviewed-by: Dan Erat <derat@chromium.org> [add] https://crrev.com/ee122ea83f2971f7dafef6fa35e6cf15df9f08dc/login_manager/child_exit_dispatcher.cc [rename] https://crrev.com/ee122ea83f2971f7dafef6fa35e6cf15df9f08dc/login_manager/child_exit_dispatcher.h [modify] https://crrev.com/ee122ea83f2971f7dafef6fa35e6cf15df9f08dc/login_manager/session_manager_service.h [rename] https://crrev.com/ee122ea83f2971f7dafef6fa35e6cf15df9f08dc/login_manager/child_exit_dispatcher_unittest.cc [modify] https://crrev.com/ee122ea83f2971f7dafef6fa35e6cf15df9f08dc/login_manager/session_manager_service.cc [delete] https://crrev.com/11f821e735d7b3af22243379a21864f2847a4d80/login_manager/child_exit_handler.cc [modify] https://crrev.com/ee122ea83f2971f7dafef6fa35e6cf15df9f08dc/login_manager/login_manager.gyp
,
May 15 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromiumos/platform2/+/27999ee8f2657657555ed37a18371cef344f2ee2 commit 27999ee8f2657657555ed37a18371cef344f2ee2 Author: Hidehiko Abe <hidehiko@chromium.org> Date: Tue May 15 04:29:49 2018 login: Clean up ChildExitHandler part 2. - Rename JobManagerInterface to ChildExitHandler. - Merge IsManagedJob into HandleExit. BUG=chromium:827933 TEST=cros_run_unit_tests --package chromeos-login. Change-Id: Ic5e58233e4bab2fb4d7e16a8db5bc5320cea6281 Reviewed-on: https://chromium-review.googlesource.com/1053931 Commit-Ready: Hidehiko Abe <hidehiko@chromium.org> Tested-by: Hidehiko Abe <hidehiko@chromium.org> Reviewed-by: Hidehiko Abe <hidehiko@chromium.org> [modify] https://crrev.com/27999ee8f2657657555ed37a18371cef344f2ee2/login_manager/child_exit_dispatcher.cc [modify] https://crrev.com/27999ee8f2657657555ed37a18371cef344f2ee2/login_manager/child_exit_dispatcher.h [modify] https://crrev.com/27999ee8f2657657555ed37a18371cef344f2ee2/login_manager/vpd_process_impl.h [modify] https://crrev.com/27999ee8f2657657555ed37a18371cef344f2ee2/login_manager/fake_container_manager.h [add] https://crrev.com/27999ee8f2657657555ed37a18371cef344f2ee2/login_manager/child_exit_handler.h [modify] https://crrev.com/27999ee8f2657657555ed37a18371cef344f2ee2/login_manager/android_oci_wrapper_unittest.cc [modify] https://crrev.com/27999ee8f2657657555ed37a18371cef344f2ee2/login_manager/child_exit_dispatcher_unittest.cc [modify] https://crrev.com/27999ee8f2657657555ed37a18371cef344f2ee2/login_manager/session_manager_service.cc [modify] https://crrev.com/27999ee8f2657657555ed37a18371cef344f2ee2/login_manager/server_backed_state_key_generator.h [modify] https://crrev.com/27999ee8f2657657555ed37a18371cef344f2ee2/login_manager/container_manager_interface.h [delete] https://crrev.com/96f358b8d502598b7d9e23e667d6235d1d79e1aa/login_manager/job_manager.h [modify] https://crrev.com/27999ee8f2657657555ed37a18371cef344f2ee2/login_manager/android_oci_wrapper.cc [modify] https://crrev.com/27999ee8f2657657555ed37a18371cef344f2ee2/login_manager/android_oci_wrapper.h [modify] https://crrev.com/27999ee8f2657657555ed37a18371cef344f2ee2/login_manager/key_generator.cc [modify] https://crrev.com/27999ee8f2657657555ed37a18371cef344f2ee2/login_manager/key_generator.h [modify] https://crrev.com/27999ee8f2657657555ed37a18371cef344f2ee2/login_manager/vpd_process_impl.cc [modify] https://crrev.com/27999ee8f2657657555ed37a18371cef344f2ee2/login_manager/key_generator_unittest.cc [modify] https://crrev.com/27999ee8f2657657555ed37a18371cef344f2ee2/login_manager/session_manager_service.h [modify] https://crrev.com/27999ee8f2657657555ed37a18371cef344f2ee2/login_manager/fake_container_manager.cc
,
Jan 7
|
|||
►
Sign in to add a comment |
|||
Comment 1 by derat@chromium.org
, Apr 2 2018Components: OS>Systems