-
Notifications
You must be signed in to change notification settings - Fork 64
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[WIP] Maintenance tasks in service #184
[WIP] Maintenance tasks in service #184
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like the direction. Look at the GitMaintenanceQueue
for directions for some of your TODOs.
string arguments = | ||
$"asuser {sessionId} {this.scalarBinPath} maintenance \"{repoRoot}\" {task} --{ScalarConstants.VerbParameters.InternalUseOnly} {this.internalVerbJson}"; | ||
|
||
ProcessResult result = this.processLauncher.LaunchProcess(ExecutablePath, arguments, repoRoot); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To allow stopping, you'll want to create a Process
object and store it as a private member. A Stop()
method would then allow killing the process, creating an exception through the CallMaintenance()
method.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@derrickstolee I plan to tackle this work as a follow up in #185
On the Mac side I think it should be pretty easy/straightforward as we're already using Process
objects.
However, on the Windows side we have a big mess of native calls that ultimately end with a call to CreateProcessAsUser
:
scalar/Scalar.Platform.Windows/CurrentUser.cs
Line 140 in 9018727
if (CreateProcessAsUser( |
And now that I take another look we're not even waiting for that process to finish
We might end up needing an interface that will internally use Process
objects on Mac, and store PROCESS_INFORMATION
on Windows (and then we can PInvoke to TerminateProcess
to stop the process).
I will also do some research and see if there is a way to create a Process
object for the process we're spawning on Windows. (GetProcessById
might be an option, but we'd have to make sure we're not introducing any races if the process exits quickly).
501d0d2
to
b7cf94b
Compare
b7cf94b
to
2dd5015
Compare
I don't know why, but when I tried to build your tip commit, my build failed because Anyway, I'm installing on my laptop to get some extra checks that things are working. |
That might be on me, I did not have a chance to confirm the latest commit built on Windows before I headed out. I'll check that first thing when I get in tomorrow. Also, I'm going to switch back to using a queue for the tasks (like we had before). The queue approach is battle tested, and it should be pretty easy to adjust the existing unit tests to work with a new queue. Plus as we discussed offline the edge case of "task taking too long" should not be a common occurrence. |
FYI - I haven't had a chance to do any manual tests yet and so things might not be working correctly yet. My plan for tomorrow:
I plan to defer the work to have the functional tests not register when cloning (at least on Mac) until we make the changes to register in clone. If any of the maintenance tests get flaky before then we can have them unmount after cloning (if they aren't already). |
Closing out this PR, it will be replaced with a non-WIP PR. |
This is extremely WIP.
Resolves #112
TODOs:
Follow up work (outside the scope of this PR):