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

Ensure tilde$1 onExit is run in correct order #13360

Merged
merged 4 commits into from
Feb 26, 2025

Conversation

masonedmison
Copy link
Contributor

@masonedmison masonedmison commented Feb 20, 2025

Resolves #13049

@masonedmison masonedmison requested a review from a team as a code owner February 20, 2025 17:37
Copy link

linux-foundation-easycla bot commented Feb 20, 2025

CLA Signed

The committers listed above are authorized under a signed CLA.

@masonedmison masonedmison force-pushed the add-finalizer-to-future branch 3 times, most recently from dbdb128 to 6b91cd1 Compare February 20, 2025 17:43
@masonedmison masonedmison marked this pull request as draft February 20, 2025 17:46
@masonedmison masonedmison force-pushed the add-finalizer-to-future branch 3 times, most recently from 0a6dbbf to 1184b41 Compare February 20, 2025 17:55
@masonedmison
Copy link
Contributor Author

If you guys are happy with this, I can make a similar change to the Akka instrumentation: the same race condition exists within its RouteConcatenation.

@masonedmison masonedmison marked this pull request as ready for review February 21, 2025 15:44
@trask
Copy link
Member

trask commented Feb 22, 2025

how hard would it be to add a test for this? thanks

@trask
Copy link
Member

trask commented Feb 22, 2025

(if you rebase, we've fixed the CI failure that you're seeing)

@masonedmison masonedmison force-pushed the add-finalizer-to-future branch from 1184b41 to 24cfb17 Compare February 22, 2025 19:46
@masonedmison
Copy link
Contributor Author

masonedmison commented Feb 22, 2025

how hard would it be to add a test for this? thanks

@trask I just played around with this for a bit and I'm having a difficult time reproducing this using straight pekko code; it might be difficult to write a failing test without introducing the tapir-pekko module as it's only with this construction of the route that the issue is triggered (at least as far I've experienced).

public static void onExit(
@Advice.Argument(value = 2) RequestContext ctx,
@Advice.Return(readOnly = false) Future<RouteResult> fut) {
fut = fut.andThen(new OnExitFinalizer(), ctx.executionContext());
Copy link
Contributor

Choose a reason for hiding this comment

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

This advice will apply also when the original method threw an exception because of the onThrowable = Throwable.class. Perhaps it would be better to write this as

    public static void onExit(
        @Advice.Argument(value = 2) RequestContext ctx,
        @Advice.Return(readOnly = false) Future<RouteResult> future,
        @Advice.Thrown Throwable throwable) {
  if (throwable != null) {
    PekkoRouteHolder.restore();
  } else {
    future = future.andThen(new OnExitFinalizer(), ctx.executionContext());
  }

I don't know how scala futures work but in java CompletableFuture the methods starting with then only apply when the future completes successfully. With CompletableFuture whenComplete would be a better choice. Please check whether andThen also runs when the future does not complete normally.

Copy link
Contributor Author

@masonedmison masonedmison Feb 24, 2025

Choose a reason for hiding this comment

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

andThen will be evaluated regardless of how the Future completes: the input of this supplied function to andThen takes a Try which models both success and failures, and the function we supply executes PekkoRouteHolder.restore() regardless of the input; e.g., we unconditionally restore regardless of how the Future completed. Importantly, andThen doesn't affect the return value of the Future and swallows any exceptions thrown within the supplied function. If I understand the above correctly, I believe this does what we want.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for confirming this.

@laurit
Copy link
Contributor

laurit commented Feb 24, 2025

how hard would it be to add a test for this? thanks

@trask I just played around with this for a bit and I'm having a difficult time reproducing this using straight pekko code; it might be difficult to write a failing test without introducing the tapir-pekko module as it's only with this construction of the route that the issue is triggered (at least as far I've experienced).

If you are reluctant to add extra dependencies you could add a separate test suite (search for registering(JvmTestSuite::class)) Suites have their own set of dependencies and are isolated from the other tests.

@masonedmison masonedmison force-pushed the add-finalizer-to-future branch 4 times, most recently from a6307fd to 2a8c4ad Compare February 24, 2025 16:06
@masonedmison
Copy link
Contributor Author

masonedmison commented Feb 24, 2025

how hard would it be to add a test for this? thanks

@trask I just played around with this for a bit and I'm having a difficult time reproducing this using straight pekko code; it might be difficult to write a failing test without introducing the tapir-pekko module as it's only with this construction of the route that the issue is triggered (at least as far I've experienced).

If you are reluctant to add extra dependencies you could add a separate test suite (search for registering(JvmTestSuite::class)) Suites have their own set of dependencies and are isolated from the other tests.

@laurit I ended up adding the tapir-pekko-http dependency as a testImplementation. With this, I was able to write a failing test (e.g., I added the test and dropped the commit that modifies the onExit logic, and the test failed; with the commit back in place, the test passes).

@masonedmison masonedmison force-pushed the add-finalizer-to-future branch from 2a8c4ad to d78981a Compare February 24, 2025 16:10
@masonedmison masonedmison requested a review from laurit February 24, 2025 16:52
@masonedmison masonedmison force-pushed the add-finalizer-to-future branch 3 times, most recently from 1fc769c to 8eac124 Compare February 24, 2025 21:49
Prior to adding the on exit finalization logic to the Future,
this test failed because of an NPE thrown because
PekkoRouteHolder.restore was called in the wrong order.
@masonedmison masonedmison force-pushed the add-finalizer-to-future branch from 8eac124 to ced90a6 Compare February 25, 2025 15:33
@trask trask merged commit f0d80b2 into open-telemetry:main Feb 26, 2025
61 checks passed
kcsurapaneni pushed a commit to kcsurapaneni/opentelemetry-java-instrumentation that referenced this pull request Feb 26, 2025
@masonedmison
Copy link
Contributor Author

@trask Do you have a sense of when this will be released?

@trask
Copy link
Member

trask commented Mar 7, 2025

Second half of next week

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.

Akka NoSuchElementException in RouteConcatenation$RouteWithConcatenation
3 participants