The Wayback Machine - https://web.archive.org/web/20200605061630/https://github.com/line/armeria/issues/2611
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

Provide more ways to run with Context #2611

Open
ikhoon opened this issue Mar 19, 2020 · 5 comments
Open

Provide more ways to run with Context #2611

ikhoon opened this issue Mar 19, 2020 · 5 comments

Comments

@ikhoon
Copy link
Contributor

@ikhoon ikhoon commented Mar 19, 2020

In our codebase, we push RequestContext and immediately run some code with try-with-resources.
For example:

try (SafeCloseable ignored = ctx.push()) {
    logger.trace(decorate(msg));
}

If RequestContext provides run(Runnable) or call(Callable) we can reduce boilerplate code and simplify it.

ctx.run(() -> logger.trace(decorate(msg));

This is inspired by gRPC Context API.
https://grpc.github.io/grpc-java/javadoc/io/grpc/Context.html#run-java.lang.Runnable-

@anuraaga
Copy link
Collaborator

@anuraaga anuraaga commented Mar 19, 2020

Seems like a nice method to add, though maybe not so helpful in our codebase specifically since we would probably still generally pick the former for better performance. Want to make sure no one picks up this issue and rewrites all the current usage at the same time as adding the method :)

@ikhoon
Copy link
Contributor Author

@ikhoon ikhoon commented Mar 19, 2020

since we would probably still generally pick the former for better performance

Good point, we create an additional Runnable object to use ctx.run().

Want to make sure no one picks up this issue and rewrites all the current usage at the same time as adding the method

I think we don't need to change the current usage in our code base. Let's just give more options to our users. 😀

@JunoJunho
Copy link
Contributor

@JunoJunho JunoJunho commented Mar 21, 2020

Is the requirement just wrapping this try clause using SafeCloseable ?

@ikhoon
Copy link
Contributor Author

@ikhoon ikhoon commented Mar 21, 2020

The RequestContext.push() already returns SafeCloseable.
I meant if a user wants to execute a function or code with RequestContext, he/she can do it by:

// 1. use makeContextAware()
// wrap the given Runnable with request context
Runnable contextAwareRunnable = ctx.makeContextAware(() -> {
    logger.trace(decorate(msg));
});
...

// can execute returned Runnable with request context later
contextAwareRunnable.run();

// 2. or use push()
// immediately push current request context to context storage such as ThreadLocal 
try (SafeCloseable ignored = ctx.push()) {
    logger.trace(decorate(msg));
}

I think we can give users another option.

Runnable r = () -> {
   // some logic should run with request context
}

ctx.run(r);

This seems a shortcut for ctx.makeContextAware(r).run(), but it can avoid creating additional Runnable.
And I think we can also support this for other functional interfaces i.e, Supplier, Callable... 😀

@JunoJunho
Copy link
Contributor

@JunoJunho JunoJunho commented Apr 16, 2020

I am still not sure why this feature is required.

This issue is for providing handy interfaces ?

You said that this can be reduced additional runnable interface creation. (Maybe reusing existing runnable instance, right?) Still, I could not come up easily the situation is.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
3 participants
You can’t perform that action at this time.