man-group / dapr-sidekick-dotnet

Dapr Sidekick for .NET - a lightweight lifetime management component for Dapr
Apache License 2.0
175 stars 21 forks source link

Slight delay after starting sidecar #40

Closed riezebosch closed 1 year ago

riezebosch commented 2 years ago

I noticed when using sidekick to start the sidecar from an integration test the sidecar was not immediately able to handle requests. This was resolved by introducing a slight delay of ~200ms.

When trying to implement a solution for this I noticed that there's a synchronous API while there's a lot of asynchrony involved.

I have a PR ready to show how I'm trying to use your library and where it doesn't fit that purpose right now.

badgeratu commented 2 years ago

First, many thanks for your interest in Sidekick! As the daprd process can conceivably go down/restart at any time, we don't provide a "wait on startup" as such but instead provide a way to check the process status so you can wait for its availability in your code. This is done via the IDaprSidecarHost.GetProcessInfo() method. So in your integration test you could do something like this:

// Wait for the host to become available
while (!sidekick.Sidecar.GetProcessInfo().IsRunning)
{
  // Dapr not yet started. Wait for 100ms.
  await Task.Delay(TimeSpan.FromMilliseconds(100));
}

While darpd is starting the GetProcessInfo() method properties look something like this:

"name": "daprd",
"id": 40924,
"version": "1.6.0",
"status": 2,
"isRunning": false,
"isAttached": false,
"description": "Dapr process 'daprd' not available, status is Starting"

I appreciate this does put the responsibility on the calling application to check the status, but that's really the case anyway when you are using Dapr (which is an external process) whether you are using Sidekick or not. We just make sure Sidekick gives you a simple way to determine the status of daprd at any time and code around it appropriately.

Also please note that the Start() methods is synchronous because under the hood we rely on System.Diagnostics.Process.Start() to start the process which doesn't have an async alternative, which means there's no point in us providing a StartAsync() method as it wouldn't be async all the way down.

So what we could maybe do is add an additional synchronous StartAndWait() method to the sidecar host, where we implement a similar loop check for the IsRunning status but allow it to be cancellable by the cancellation token passed into the method (or allow a maximum wait time). So you could then do:

// Create a cancellation token, this could be signaled on a separate thread.
var cancellationToken = new DaprCancellationToken();

// Start Sidekick and wait synchronously for it to reach the Running state. 
// Can be canceled either by signaling cancellationToken or if the timeout expires.
sidekick.Sidecar.StartAndWait(
  () => new DaprOptions
  {
    Sidecar = new DaprSidecarOptions { ComponentsDirectory = "components", DaprGrpcPort = 3001 }
  }, cancellationToken, timeout: TimeSpan.FromMilliseconds(1000));

Would that help?

riezebosch commented 2 years ago

I didn't know about sidekick before I read this blog: https://markheath.net/post/running-locally-with-dapr-options. but I implemented my own solution to manage the self-hosted sidecar from an integration test: https://github.com/riezebosch/dapr.wrapr

There I scan the actual output of the sidecar to make sure it is up and running. Another benefit it has is that I made it disposable to make the cleanup easier.

I understand that it is a slightly different use case than you have/had but I was happy to see it was quite frictionless to drop in your (battle-tested) solution.

badgeratu commented 2 years ago

Nice 👍 I suspect we've come up with the same solution for detecting when the sidecar is actually running, as we also monitor the log messages from stdout to determine if the sidecar is ready to accept calls - its this status change that controls the value of sidekick.Sidecar.GetProcessInfo().IsRunning.

In summary then, do you still need a change here or are you ok to monitor the IsRunning property to determine the state of the sidecar?

riezebosch commented 2 years ago

Not too sure. If you think about supporting the use of this library in a testing context it would make sense to fix it in one place. Maybe a separate method (or even an extension method) to await the sidecar until it is ready. In the meantime, I'll stick to my own package since it is geared a bit more towards short-lived sidecars and cleanup on disposal.

Feel free to close this issue or takeover the PR and do whatever you like.