Skip to content

Fix Stackdriver's shutdown behavior #4358

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.

Sign up for GitHub

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

Merged
merged 4 commits into from
Jan 31, 2024
Merged

Fix Stackdriver's shutdown behavior #4358

Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
0bbe0bb
Do not close the client before close has finished as close publishes …
dimovelev Nov 17, 2023
3dd1e8b
format the code as expected
dimovelev Nov 17, 2023
e80d556
readd the method as it seems to break some test if missing
dimovelev Nov 17, 2023
9a80c05
close the client upon start if necessary so that multiple stop/start …
pb-dimo Dec 12, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
51 changes: 49 additions & 2 deletions ...egistry-stackdriver/src/main/java/io/micrometer/stackdriver/StackdriverMeterRegistry.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -125,6 +125,7 @@ public void start(ThreadFactory threadFactory) {
logger.error("unable to start stackdriver, service settings are not available");
}
else {
shutdownClientIfNecessary(true);
Copy link
Member

Choose a reason for hiding this comment

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

I don't think this is necessary if nothing is done with the client in stop. The question is: should something be done in stop or only in close? We haven't been very clear about what should happen where (no JavaDocs). I'll open a separate issue to clear that up.

It's a bit wasteful of resources to not shutdown the client when a user calls stop on the registry, but in terms of publishing it shouldn't be an issue because the scheduled call to publish won't happen after stop is called until start is called again. Looking at other implementations, we do the same in WavefrontMeterRegistry - nothing special in stop and close the client on close after super.close().

Copy link
Member

Choose a reason for hiding this comment

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

I don't think this is necessary if nothing is done with the client in stop.

I remembered now the issue I was pointing out in my previous review. We don't want to replace the client without shutting down the old one and making it eligible for garbage collection. Therefore having this line here should prevent that.

Copy link
Member

Choose a reason for hiding this comment

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

As promised, I opened #4632 to track clarifying stop semantics.

try {
this.client = MetricServiceClient.create(metricServiceSettings);
super.start(threadFactory);
Expand All @@ -138,11 +139,57 @@ public void start(ThreadFactory threadFactory) {

@Override
public void stop() {
if (client != null)
client.shutdownNow();
Copy link
Member

Choose a reason for hiding this comment

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

Removing this from here will potentially be problematic (a resource leak) if the registry is stopped and started multiple times. If it is only stopped as part of close then I think this fix works. We should have some kind of tests for this, ideally.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

  • added a shutdown and close when start is called
  • I could not find any emulator etc. so that automated integration tests could be done against it. I have tested it manually against my GCP environment though.

super.stop();
}
Comment on lines 140 to 143
Copy link

Choose a reason for hiding this comment

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

Is it necessary to have an overridden method that just calls its super()?
If we remove this method the stop() from parent is called anyway.

Copy link
Member

@shakuzen shakuzen Jan 31, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Thanks for taking a look. Indeed we don't need to override if we aren't doing anything but calling the super here. I'll take care of it in a polish commit.

Copy link
Member

Choose a reason for hiding this comment

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

Ah, it looks like removing the override breaks binary compatibility even if it is source compatible, which @dimovelev found out when they tried to remove it and added it back.


@Override
public void close() {
try {
super.close();
}
finally {
shutdownClientIfNecessary(false);
}
}

protected void shutdownClientIfNecessary(final boolean quietly) {
if (client != null) {
if (!client.isShutdown()) {
try {
client.shutdownNow();
final boolean terminated = client.awaitTermination(10, TimeUnit.SECONDS);
if (!terminated) {
logger.warn("The metric service client failed to terminate within the timeout");
}
}
catch (final RuntimeException e) {
if (quietly) {
logger.warn("Failed to shutdown the metric service client", e);
}
else {
throw e;
}
}
catch (final InterruptedException e) {
Thread.currentThread().interrupt();
return;
}
}
try {
client.close();
}
catch (final RuntimeException e) {
if (quietly) {
logger.warn("Failed to close metric service client", e);
}
else {
throw e;
}
}
client = null;
}
}

@Override
protected void publish() {
if (client == null) {
Expand Down