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

Thread locals are lost after the control is returned from SDK methods to the client code #5242

Open
2 tasks
electronic-dk opened this issue May 23, 2024 · 6 comments
Labels
feature-request A feature should be added or improved. needs-triage This issue or PR still needs to be triaged.

Comments

@electronic-dk
Copy link

Describe the feature

This is pretty much a copy of this issue #2142 to be able to continue the discussion.
It's been closed for a while, so I guess it's not actively monitored anymore even though the issue still exists.

Basically the issue is that since aws sdk v2 uses netty nio, all thread locals are lost after any async sdk method is completed, that includes spring security context, mdc, servlet request attributes or anything else the developer might have put into a thread local. Unfortunately, there seems to be no way to tell the sdk to preserve this context or extend the sdk in a way that makes it possible. This makes it really hard to use aws sdk v2 in a spring web mvc environment without resorting to migrating to kotlin coroutines or some hacky workarounds (e.g. aspects).
Specifying SdkAdvancedAsyncClientOption.FUTURE_COMPLETION_EXECUTOR is also not a solution, since this executor is used after the control has returned from the netty thread pool so the context is already lost at this point.

Use Case

Logging and tracing in a Spring Web MVC environment. Trace and span ids are stored as MDC context (ThreadLocal variables). It's also common to put app-specific data into MDC context to be able to match log lines to a specific request. Currently, all this data are lost after any SDK call, so tracing essentially breaks after an AWS SDK call.

Proposed Solution

No response

Other Information

A shaky workaround I ended up using is to have an aspectj aspect around AWS SDK methods that adds a CompletableFuture handler that restores Thread Locals.

Acknowledgements

  • I may be able to implement this feature request
  • This feature might incur a breaking change

AWS Java SDK version used

2.25.43

JDK version used

17.0.6

Operating System and version

macOS Sonoma 14.5

@electronic-dk electronic-dk added feature-request A feature should be added or improved. needs-triage This issue or PR still needs to be triaged. labels May 23, 2024
@electronic-dk
Copy link
Author

I've filed this one as a feature request and not a bug, as I wasn't sure if Thread Locals support was even considered initially. But the issue can be converted into a bug if you think it gives more clarity.

OS version does not really matter, the issue presents on all OS that Java supports. It can also be observed on Java 11 and 21.

@scrocquesel
Copy link
Contributor

@debora-ito Would you mind consider this issue for triage ?

@bdeweer
Copy link

bdeweer commented Dec 2, 2024

@scrocquesel Facing the same issue with a quarkus application.

@Path("/exec")
@GET
@Produces(MediaType.TEXT_PLAIN)
public Uni<String> exec() {

    MDC.put("mykey", "myvalue");

    return
            S3Utils
                    .getObject(s3Context) //returns Uni<ResponseBytes<GetObjectResponse>> 
                    .invoke(() -> log.info("bug MDC : {}", MDC.get("mykey"))) //null
                    .chain(() -> Uni.createFrom().item("MDC fails"));
    }

2024-12-02 22:46:23 INFO [org.acme.GreetingResource] (sdk-async-response-13-0) ({}) bug MDC : null

Adding :

.asyncConfiguration(b -> b.advancedOption(SdkAdvancedAsyncClientOption.FUTURE_COMPLETION_EXECUTOR, executor)) //executor is Injected by CDI : @Inject ManagedExecutor executor`

doesn't change anything :

2024-12-02 22:46:52 INFO [org.acme.GreetingResource] (executor-thread-1) ({}) bug MDC : null

Both sdk-async-response-13-0 and executor-thread-1 threads fail to propagate the MDC.

The quarkus documentation supports it
Can you help me?

@scrocquesel
Copy link
Contributor

With quarkus and mutiny, see the pattern in this comment quarkiverse/quarkus-amazon-services#998 (comment).
This is a workaround and I'm looking forward this issue to help removing this explicit context switching without having to create a new client instance each time we want to capture the context and provide a custom future completion executor.

@bdeweer
Copy link

bdeweer commented Dec 2, 2024

Thank you

Im doing like this :

@Path("/exec")
    @GET
    @Produces(MediaType.TEXT_PLAIN)
    public Uni<String> exec() {

        MDC.put("mykey", "myvalue");

        //var context = VertxContext.getOrCreateDuplicatedContext();
        var context = Vertx.currentContext();
        return
                S3Utils
                        .getObject(s3Context)
                        //.emitOn(r -> context.runOnContext(_ -> r.run()))
                        .emitOn(context::runOnContext)
                        .invoke(() -> log.info("fixing MDC : {}", MDC.get("mykey"))) //now ok :)
                        .chain(() -> Uni.createFrom().item("MDC fixed"));
    }

Are the two approaches the same?

@scrocquesel
Copy link
Contributor

In this case yes. You'll always have a valid current context

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature-request A feature should be added or improved. needs-triage This issue or PR still needs to be triaged.
Projects
None yet
Development

No branches or pull requests

3 participants