feat: PyroscopeAgent.stop #149
Conversation
There was a problem hiding this comment.
Looks good overall, I left a few minor remarks.
Something to think about now (and maybe add separately later) is how users interact with the start / stop methods. Right now, they do not return anything or throw exceptions which means user code has no way of knowing if they succeeded or not and whether one can continue interacting with the API.
| logger.log(Logger.Level.ERROR, "Failed to start profiling - already started"); | ||
| return; | ||
| } | ||
| if (sFailed) { |
There was a problem hiding this comment.
This could be a bit restrictive if we have intermittent failures (one attempt to start fails but the next one could succeed). We could keep it this way though and make changes if this becomes relevant in the future.
There was a problem hiding this comment.
at first I thought it was a good idea, but then while writing a test I immediately stumbled into this check , with no way around.
I removed sFailed check alltogether.
Thanks
| return; | ||
| } | ||
| if (sFailed) { | ||
| logger.log(Logger.Level.INFO, "Not start profiling due to recent failure"); |
There was a problem hiding this comment.
Just a nit, maybe this should be logged at ERROR level for consistency with the above? Also, "Will not start profiling due to recent failure" might be more clear.
There was a problem hiding this comment.
changed to ERROR
| logger.log(Logger.Level.INFO, "Profiling started"); | ||
| } catch (final Throwable e) { | ||
| sFailed = true; | ||
| logger.log(Logger.Level.ERROR, "Error starting profiler %s", e); |
There was a problem hiding this comment.
we should probably set sOptions to null here, otherwise a subsequent start would say "profiling already started" which is not the case
There was a problem hiding this comment.
agree, set to null
| public static void stop() { | ||
| synchronized (sLock) { | ||
| if (sOptions == null) { | ||
| return; |
There was a problem hiding this comment.
perhaps add a log statement here for consistency?
There was a problem hiding this comment.
the problem here is that we dont have sOptions.logger, so we have to write to stderr, which is not ideal.
Added DefaultLogger.PRECONFIG_LOGGER.Log
| return; | ||
| } | ||
| try { | ||
| sOptions.scheduler.stop(); |
There was a problem hiding this comment.
we could add a log statement here as well, to know that the profiler was stopped
| if (svc != null) { | ||
| awaitTermination(svc); | ||
| } |
There was a problem hiding this comment.
Do we want this inside the synchronized block? Otherwise we can have another executor active while the old one is shutting down (which is maybe fine?)
There was a problem hiding this comment.
I've left the comment in the code, and copypaste it here as well
// shutdown here not under lock to avoid deadlock ( the task may block to wait for lock and
// we are holding the lock and waiting for task to finish)
// There is still synchronization happens from the PyroscopeAgent class,
// so there are no concurrent calls to start/stop. So there is no lock here
Does it make sense?
There was a problem hiding this comment.
You are right, the synchronization in PyroscopeAgent will prevent the scenario I described. Someone could potentially misuse this class in the future, but it is unlikely and the impact is low.
| stopSchedulerLocked(); | ||
| this.executor = null; | ||
| } | ||
| if (svc != null) { |
There was a problem hiding this comment.
if we need this check here, we probably need it inside stopSchedulerLocked() since we access the same reference there.
There was a problem hiding this comment.
I removed the nil check. I think it is not needed.
Because of the logic in PyroscopeAgent, if the scheduler.start() was not successful or PyroscopeAgent.Start() was not called at all, then scheduler.stop would never be called, so if we get a call to scheduler.stop, then we have an executor
I would appreciate if you could confirm or deny :D
There was a problem hiding this comment.
I've also removed null check on job.cancel
| this.executor = Executors.newSingleThreadScheduledExecutor(THREAD_FACTORY); | ||
| this.job = executor.scheduleAtFixedRate(this::schedulerTick, | ||
| firstProfilingDuration.toMillis(), config.uploadInterval.toMillis(), TimeUnit.MILLISECONDS); | ||
| this.started = true; |
There was a problem hiding this comment.
perhaps a logging statement here could be helpful
| throw new IllegalStateException(throwable); | ||
| logger.log(Logger.Level.ERROR, "Error stopping profiler %s", throwable); |
There was a problem hiding this comment.
This used to throw but now it doesn't, is this intentional?
There was a problem hiding this comment.
not sure I understand, there were no stop functionality before, so it did not throw.
However I see the inconsistency, start throws IllegalStateException on profiler.start fail, and stop does not. I will try to make it consistent.
There was a problem hiding this comment.
Made stopSchedulerLocked to rethrow IllegalStateException in case this.profiler.stop(); fails.
Moved a bunch of logic to finally blocks.
PTAL
There was a problem hiding this comment.
My bad, the GitHub diff fooled me but you got my concern anyway :)
|
|
||
| @Override | ||
| public void stop() { | ||
| ScheduledExecutorService svc = this.executor; |
There was a problem hiding this comment.
this field reading should be under lock
There was a problem hiding this comment.
moved under lock
|
@aleks-p Thanks for review! I addressed your comments and also added a small test |
| stopSchedulerLocked(); | ||
| this.executor = null; | ||
| } | ||
| if (svc != null) { |
| throw new IllegalStateException(throwable); | ||
| logger.log(Logger.Level.ERROR, "Error stopping profiler %s", throwable); |
There was a problem hiding this comment.
My bad, the GitHub diff fooled me but you got my concern anyway :)
| if (svc != null) { | ||
| awaitTermination(svc); | ||
| } |
There was a problem hiding this comment.
You are right, the synchronization in PyroscopeAgent will prevent the scenario I described. Someone could potentially misuse this class in the future, but it is unlikely and the impact is low.
No description provided.