Fix incorrect exit code for asprof when running in collect mode by Baraa-Hasheesh · Pull Request #1623 · async-profiler/async-profiler

Baraa-Hasheesh · 2025-12-15T10:42:54Z

Description

This change tackles one of the race conditions described in #1622

Being the race condition of what will execute first, the stop at the end of the collect command or the VMDeath hook

If VMDeath is executed first, then it will stop the profiler, dump the profile & proceed to terminate the profiler,
Following that the stop command will fail as the profiler is terminated

In this change, an internal flag is passed from asprof to the profiler, as to mark the current stop command as one coming from collect (Other stops commands should still fail) as to give it special handling

Related issues

#1622

Motivation and context

asprof shouldn't have a non zero exit code from VMDeath

How has this been tested?

manual testing

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

apangin · 2025-12-15T14:06:13Z

                _inverted = true;

+            CASE("_collect")
+                _collect = true;


No "secret" options please.

The problem I kept hitting, is that profiler has different behavior depending on various condition, I wanted to keep the behaviors untouched as is,

For example, the following won't generate a log

asprof start -cpu -f out.html 1234 asprof stop 1234

while, this will generate a log

asprof start -cpu -f out.html 1234 VMDeath asprof stop 1234

so a "secret" made sense to not effect existing behaviors

asprof is nothing but a binary script that loads profiler library in a target process and invokes commands on it. It could as well be a shell script or a custom program that does a similar thing. And I know people use custom scripts. A hidden option in asprof will not help them.

I've updated my approach to be determined on the usage,

If the user starts the asprof with a file, then it's fine for the stop call to fail on termination & that failed stop can be considered a success (the VMDeath would have done the dump for the user)

So the user can address this race in any custom script by providing the output file in both start & stop as the asprof collect is currently doing ee54ad2

apangin · 2025-12-15T14:12:06Z

 Error Profiler::dump(Writer& out, Arguments& args) {
    MutexLocker ml(_state_lock);
-    if (_state != IDLE && _state != RUNNING) {
+    if (args._collect && _state == TERMINATED) {


OK, stopping profiler in TERMINATED state is not an error. But why should behavior differ when running asprof standalone vs. from a script? And why is this check in dump, if asprof invokes stop?

It's mostly my personal interpretation on how the profiler should behave

And why is this check in dump, if asprof invokes stop

The fall through from stop to dump makes having this check in stop redundant

The only edge case would be if user called stop with no output file, but then it will be reported as a failure with a clear error Profiler is not active

why should behavior differ when running asprof standalone vs. from a script

In asprof "collect" mode both start & end are called with output file, which means we're guaranteed for the profiler to dump the profile on disk via VMDeath + global args

However for a regular start/stop commands, the dump isn't guaranteed which is arguably an error

asprof start -e cpu 915 asprof stop -f out.html

Again, this is not specific to asprof; asprof is just a wrapper.
It should be a decision of libasyncProfiler to return an error or not.

I've updated my approach to be determined on the usage,

If the user starts the asprof with a file, then it's fine for the stop call to fail on termination & that failed stop can be considered a success (the VMDeath would have done the dump for the user)

So the user can address this race in any custom script by providing the output file in both start & stop as the asprof collect is currently doing ee54ad2

apangin · 2025-12-15T20:07:15Z


 static ProfilingWindow profiling_window;

+static const char* _start_file = NULL;


This is unnecessary. If _start_file comes from _global_args, then you can access it via _global_args. But if it comes from elsewhere, the pointer will be invalid.

that make sense

apangin · 2025-12-15T20:30:48Z

 Error Profiler::dump(Writer& out, Arguments& args) {
    MutexLocker ml(_state_lock);
-    if (_state != IDLE && _state != RUNNING) {
+    if (_state == TERMINATED && _start_file != NULL && args.rawFile() != NULL && strcmp(_start_file, args.rawFile()) == 0) {


This is a bit better than _collect, but is still a hack. I was thinking of something simpler, like always returning Error::OK for stop command if we are in TERMINATED. Although this is not fully accurate, I can't imagine a case when this is a real problem.

Alright, what about this hack:

if (_state == TERMINATED && args.hasTemporaryLog()) { return Error::OK; }

The effect is the same as in your first revision, just with less code changes.

like always returning Error::OK for stop command if we are in TERMINATED

The real problematic part is the dump command not the stop & the dump will fail if profiler is terminated (Not safe to access JNI & JVMTI)

IMO if the output selected by the user isn't dumped, then that should be considered a failure, what are you thoughts on this @apangin

From this perspective the only way for the profiler to dump it correclty, is for the user to provide the output file on the start command, hence my hack

Alright, what about this hack:

if (_state == TERMINATED && args.hasTemporaryLog()) {
return Error::OK;
}
The effect is the same as in your first revision, just with less code changes.

This will also mark normal stops as a success even if the profiler didn't really write the profiles, which is wrong

asprof start -e cpu 1234 sleep 10 asprof stop -f out.html 1234 <--- this hit the race condition & unable to generate the out.html

OK, agreed - let's proceed with the file trick as I don't see a simple way to solve this cleanly without redesigning the entire protocol.
A good news is that it will be a tiny local change: all you need is to compare args._file to _global_args._file.

handled here 2c1c53d

_collect should not fail due to VMDeath after profiler start

613cc26

apangin reviewed Dec 15, 2025

View reviewed changes

Baraa-Hasheesh added 2 commits December 15, 2025 19:38

fix by checking on _file provided in start

ee54ad2

flip checks

4ae2df2

apangin reviewed Dec 15, 2025

View reviewed changes

simplify

2c1c53d

apangin merged commit a77d091 into async-profiler:master Dec 16, 2025
27 checks passed

Baraa-Hasheesh mentioned this pull request Dec 16, 2025

asprof collect mode is quite sensitive to race conditions related to VMDeath #1622

Open


		static ProfilingWindow profiling_window;

		static const char* _start_file = NULL;

Conversation

Baraa-Hasheesh commented Dec 15, 2025

Description

Related issues

Motivation and context

How has this been tested?

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Baraa-Hasheesh Dec 15, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Baraa-Hasheesh Dec 15, 2025 •

edited

Loading