fix(agent): add logic to start Gateway service after update #1172
Conversation
Let maintainers know that an action is required on their side
|
| use crate::Error; | ||
|
|
||
| pub struct ServiceManager { | ||
| handle: Owned<SC_HANDLE>, |
There was a problem hiding this comment.
praise: Nice, I didn’t notice about these wrappers until now. We should definitely use Ref and Owned more!
| Ok(Self { handle }) | ||
| } | ||
|
|
||
| fn open_service_with_access(&self, service_name: &str, access: u32) -> anyhow::Result<Service> { |
There was a problem hiding this comment.
Thought: Error type in this library should be refactored and used consistently across all modules, falling back to anyhow for now (I need to throw AllocError from RawBuffer somehow, mapping it to some win32 error code feels wrong).
There was a problem hiding this comment.
How are you thinking we should go about this? I don’t think it’s necessarily better to have a single Error type which can be anything, but there is no easy way to create lazy enums like in TypeScript to combine multiple errors easily either :/
We may want to provide a top-level Error type for complex errors:
enum Error {
Alloc(AllocError),
System(windows::core::Error),
Other(anyhow::Error),
…
}Well, quite similar to what we already have in the end 😂
Is it something like that you were thinking about?
There was a problem hiding this comment.
The problem is that crate uses a mix of our own Error type (e.g in acl.rs) but in many places anyhow is used (e.g. token.rs). I think we should use a single Error type throughout all crate modules instead of this mix. IMHO anyhow should only be used in top-level crates like devolutions-gateway/devolutions-agent (exception - if third party library throws anyhow, wrap it in its enum variant.
What is really needed in this crate is to eliminate the mixing of error types in library API, but this is definitely out of the scope of PR.
However, I don't know why I decided to switch to anyhow instead of adding new Error variant before going on vacation, thank you for bringing that up, looks like my brain was very blurry during those last-minute changes, I'll look into it again, most likely switch to Error back 😄
There was a problem hiding this comment.
Okay, yeah we could stop using anyhow though I would like some way to attach additional context to the error. Sometimes we just get an obscure system error in the Error::System variant with no idea of which call caused it.
Given that we typically don’t inspect programmatically the structured errors, I think surprisingly anyhow is kind of fine even in libraries, though I tend to agree with you; it’s cleaner to have a little bit more of structure for libraries.
I don’t think we should necessarily have a strict rule of "one crate = one Error type" though. The standard library is not following this rule, because errors returned by a given module are often structurally completely unrelated to the kind of errors returned in another module.
We do have that in win-api-wrappers as well: we don’t need to use a wider error in the raw_buffer module, because AllocError is all we need. Everything is cleanly encapsulated inside the module.
Your service module could also just expose a ServiceError that is self-explanatory enough and does not require an additional context string to it:
enum ServiceError {
#[error("failed to query service state for `{product}`")]
QueryServiceState {
product: Product,
source: win_api_wrappers::Error,
},
#[error("failed to start service for `{product}`")]
StartService {
product: Product,
source: win_api_wrappers::Error,
},
}Just an idea, maybe you need to add a few variants, and maybe it’s just not a good idea at all in this case.
FYI: I think we should split the win-api-wrappers into two crates: one for the cross-platform primitives (the code that we use to interface with the Windows API, but that is still pure Rust), such as RawBuffer, Win32Dst and other helpers, and one for the code actually wrapping Windows API such as token module. I’m thinking about doing this refactor a little bit later this week. The code in the helper crate would not have a single top-level error because everything is only loosely related, but a top-level error in the API wrapper crate would be very useful.
We could imagine something similar to what we have in IronRDP: https://github.com/Devolutions/IronRDP/blob/25bbb2682c95419fc18a61cab81e29a0ab9f23d4/crates/ironrdp-error/src/lib.rs
This should be adjusted to our needs. First no generic type, we can hardcode the enum. Also maybe we need an optional context string for only system errors:
enum Error {
Alloc(AllocError),
System { source: windows::core::Error, context: Option<String> },
Other(anyhow::Error),
…
}If you want to go ahead and design that, let me know your ideas in Slack, as the conversation here is getting buried by GitHub UI 😂
There was a problem hiding this comment.
Oh, by the way, this didn’t occur to me either when I initially reviewed, but you don’t necessarily need to propagate the AllocError errors. In the dst module, we assume there is enough memory to allocate the structure:
(pretty much like Vec is doing)
And we make some reasonable assumptions about the number of elements:
There was a problem hiding this comment.
Hmm, I thought about it for a bit and I think having one crate=one error type error is not optimal either...
I like the idea of per-module custom error types. That way the error types will have only minimal nesting (easier to match for specific errors if needed) while keeping the possibility to add more context on-demand and new error kinds without breaking the API (Significantly at least. I think we could make all custom errors #[non_exhaustive] to enforce error matching with catch-all but I don't know if it worth it?).
For the scope of this PR, I'll add a new ServiceError type with a single ServiceError::Win32 variant (I'll follow your suggestion and add .expect for OOM)
There was a problem hiding this comment.
Sounds good! For the record, I think it was a good thing to have variants based on the kind of failure like you initially had when the PR was opened:
enum ServiceError {
#[error("failed to query service state for `{product}`")]
QueryServiceState {
product: Product,
source: win_api_wrappers::Error,
},
#[error("failed to start service for `{product}`")]
StartService {
product: Product,
source: win_api_wrappers::Error,
},
}Instead of win_api_wrappers::Error, this could be windows::core::Error:
enum ServiceError {
#[error("failed to query service state for `{product}`")]
QueryServiceState {
product: Product,
source: windows::core::Error,
},
#[error("failed to start service for `{product}`")]
StartService {
product: Product,
source: windows::core::Error,
},
}The source is windows::core::Error for each of them, but the context is different.
I’ll leave the details up to you. You may merge with the single variant too if you prefer.
There was a problem hiding this comment.
To be honest, I think it makes sense only in the higher-level crates - e.g. I still use Product in devolutions-agent/src/updater/error.rs to distinguish errors for different products in updater module, but win-api-wrapper errors are usually wrapped in higher level errors and passed as source
There was a problem hiding this comment.
Indeed, there are not many FFI calls interleaved. At this point you may as well return a windows::core::Error directly.
|
@CBenoit I am ready for review round 2 😃 |
|
@CBenoit please review again |
|
Sure! Can you also answer to my questions in the previous comments such as here? #1172 (comment) |
|
About this suggestion: #1172 (comment) |
Ah sorry, I completely missed them |
This PR adds the following logic to the updater: