Skip to content
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

Allow JIT killing of Raven.Embedded.EmbeddedServer by tearing it down as part of stop sequence #4824

Open
wants to merge 8 commits into
base: logging-improvements
Choose a base branch
from

Conversation

ramonsmits
Copy link
Member

Teardown Raven.Embedded.EmbeddedServer as part of stop sequence so that we kill the instance if needed as adhering to cancellation is not possible in host builder root container dispose.

…that we kill the instance if needed as adhering to cancellation is not possible in host builder root container dispose.
@andreasohlund andreasohlund marked this pull request as ready for review February 19, 2025 11:14
Comment on lines +198 to +199
Task.Run(() => EmbeddedServer.Instance.Dispose(), cancellationToken),
waitForCancellationTask
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ramonsmits I am trying to understand the reason for this code.
I thought we wanted to give a chance for RavenDB server to end on its own, and then if it takes a long time, then we resort to killing it, or am I misunderstanding?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought we wanted to give a chance for RavenDB server to end on its own, and then if it takes a long time, then we resort to killing it, or am I misunderstanding?

@johnsimons that is exactly what this code should do:

  • Launch the dispose as a task, and the timer
  • Wait for either to complete
  • If its the timer task it ran out of time and we kill the child process

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I may be wrong here @ramonsmits, but wouldn't in that case there would be an issue with passing the same cancellationToken token to both tasks.
For example, the cancellation token is triggered, so both tasks (the Delay and the Raven process will receive that cancellation), the Delay task will throw an exception (CanceallationException, I can't really remember), but the Raven process may take 30 secs to finish off, so the chances of the Delay being first are quite high IMO, right?

@johnsimons
Copy link
Member

I was just looking at what EmbededServer is, and it seems to be just a wrapper for the RavenDB server process.
Which makes me wonder if we could do a better job by just implementing that code ourselves.
Has this been considered at all?

@ramonsmits
Copy link
Member Author

I was just looking at what EmbededServer is, and it seems to be just a wrapper for the RavenDB server process. Which makes me wonder if we could do a better job by just implementing that code ourselves. Has this been considered at all?

@johnsimons I have been looking at that too:

We could clone that code into our code base but it would just be better if they would have a client.

@johnsimons
Copy link
Member

We could clone that code into our code base but it would just be better if they would have a client.

Do we even need to do that, like I said it is just a wrapper around Process.Start .... Process.End, TBH, not very useful

Comment on lines +192 to +220
// TODO: await EmbeddedServer.Instance.StopAsync(cancellationToken);

var processId = await EmbeddedServer.Instance.GetServerProcessIdAsync(cancellationToken);

var waitForCancellationTask = Task.Delay(Timeout.Infinite, cancellationToken);
var firstTask = await Task.WhenAny(
Task.Run(() => EmbeddedServer.Instance.Dispose(), cancellationToken),
waitForCancellationTask
);

if (firstTask == waitForCancellationTask)
{
try
{
Logger.WarnFormat("Killing RavenDB server PID {0} child process because host cancelled", processId);
var ravenChildProcess = Process.GetProcessById(processId);
ravenChildProcess.Kill(entireProcessTree: true);
// Kill only signals
Logger.WarnFormat("Waiting for RavenDB server PID {0} to exit... ", processId);
await ravenChildProcess.WaitForExitAsync(CancellationToken.None);
}
catch (Exception e)
{
Logger.ErrorFormat("Failed to kill RavenDB server PID {0}\n{1}", processId, e);
}
}

serverOptions = null!;
Logger.Debug("Stopped RavenDB server");
Copy link
Member

@johnsimons johnsimons Feb 26, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I found the usage of the Task.Delay with infinite time a bit confusing.
So I wonder if something like this would still work and be simpler, thoughts @ramonsmits ?

Suggested change
// TODO: await EmbeddedServer.Instance.StopAsync(cancellationToken);
var processId = await EmbeddedServer.Instance.GetServerProcessIdAsync(cancellationToken);
var waitForCancellationTask = Task.Delay(Timeout.Infinite, cancellationToken);
var firstTask = await Task.WhenAny(
Task.Run(() => EmbeddedServer.Instance.Dispose(), cancellationToken),
waitForCancellationTask
);
if (firstTask == waitForCancellationTask)
{
try
{
Logger.WarnFormat("Killing RavenDB server PID {0} child process because host cancelled", processId);
var ravenChildProcess = Process.GetProcessById(processId);
ravenChildProcess.Kill(entireProcessTree: true);
// Kill only signals
Logger.WarnFormat("Waiting for RavenDB server PID {0} to exit... ", processId);
await ravenChildProcess.WaitForExitAsync(CancellationToken.None);
}
catch (Exception e)
{
Logger.ErrorFormat("Failed to kill RavenDB server PID {0}\n{1}", processId, e);
}
}
serverOptions = null!;
Logger.Debug("Stopped RavenDB server");
var processId = await EmbeddedServer.Instance.GetServerProcessIdAsync(cancellationToken);
try
{
await Task.Run(() => EmbeddedServer.Instance.Dispose(), cancellationToken);
}
catch (OperationCanceledException) when (shutdownCancellationToken.IsCancellationRequested)
{
try
{
Logger.WarnFormat("Killing RavenDB server PID {0} child process because host cancelled", processId);
var ravenChildProcess = Process.GetProcessById(processId);
ravenChildProcess.Kill(entireProcessTree: true);
// Kill only signals
Logger.WarnFormat("Waiting for RavenDB server PID {0} to exit... ", processId);
await ravenChildProcess.WaitForExitAsync(CancellationToken.None);
}
catch (Exception e)
{
Logger.ErrorFormat("Failed to kill RavenDB server PID {0}\n{1}", processId, e);
}
}
serverOptions = null!;
Logger.Debug("Stopped RavenDB server");

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants