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

Schedulers.isInNonBlockingThread() should return true on EventLoop #2404

Closed
minwoox opened this issue Jan 16, 2020 · 4 comments
Closed

Schedulers.isInNonBlockingThread() should return true on EventLoop #2404

minwoox opened this issue Jan 16, 2020 · 4 comments
Labels
Milestone

Comments

@minwoox
Copy link
Member

minwoox commented Jan 16, 2020

In #1665, we made our EventLoop implement NonBlocking interface so that a user who uses Project Reactor gets a warning log when he/she is using a blocking call on our EventLoops.
We didn't use NonBlocking in Project Reactor, but added it under our own reactor.core.scheduler directory because we didn't want to add the Reactor dependency to the core.
However, it doesn't seem to work so we should fix it.

Sample snippet from @joonhaeng:

@Slf4j
@RestController
public class DemoController {
    @RequestMapping("/test")
    public String test() {
        log.debug("{}",Thread.currentThread().getName());
        log.debug("{}",Thread.currentThread().getClass());
        String test[] = new String[] {"1", "2", "3", "4", "5"};
        return Flux.fromArray(test).blockFirst(); // A warning log should appear.
    }
}
@anuraaga
Copy link
Collaborator

This looks like it was a proguard issue up to 0.97.0. The NonBlocking interface is empty and wasn't used anywhere in our code so got stripped away as being a no-op. If you look at the published JAR, it doesn't have the reactor package and the bytecode for EventLoopThread does not reference NonBlocking.

But the issue should already be fixed at HEAD - now we use the NonBlocking interface in our own code

https://github.com/line/armeria/blob/master/core/src/main/java/com/linecorp/armeria/internal/eventloop/EventLoopCheckingUtil.java#L47

so proguard keeps it.

@minwoox
Copy link
Member Author

minwoox commented Jan 16, 2020

Ah, I couldn't even imagine that. Thanks!

@ikhoon
Copy link
Contributor

ikhoon commented Jan 16, 2020

Thanks for the inspection! The mystery is solved. 🕵️‍♀️

@ikhoon ikhoon added the defect label Jan 16, 2020
@ikhoon ikhoon added this to the 0.98.0 milestone Jan 16, 2020
@ikhoon
Copy link
Contributor

ikhoon commented Jan 16, 2020

Already Fixed by #2275

@minwoox minwoox closed this as completed Jan 20, 2020
trustin added a commit to trustin/armeria that referenced this issue Jan 20, 2020
Related: line#2404
Motivation:

We allowed Proguard to prune all classes that are not under
`com.linecorp.armeria` (except the internal ones). This can make our own
forked tag interface `reactor.core.scheduler.NonBlocking` to be pruned
by ProGuard, although it is currently kept indirectly and automatically.
It'll be safer if we configure ProGuard explicitly.

Modifications:

- Add ProGuard `keep` for `reactor.core.scheduler.NonBlocking`

Result:

- No chance of regression
trustin added a commit that referenced this issue Jan 20, 2020
Related: #2404
Motivation:

We allowed Proguard to prune all classes that are not under
`com.linecorp.armeria` (except the internal ones). This can make our own
forked tag interface `reactor.core.scheduler.NonBlocking` to be pruned
by ProGuard, although it is currently kept indirectly and automatically.
It'll be safer if we configure ProGuard explicitly.

Modifications:

- Add ProGuard `keep` for `reactor.core.scheduler.NonBlocking`

Result:

- No chance of regression
fmguerreiro pushed a commit to fmguerreiro/armeria that referenced this issue Sep 19, 2020
fmguerreiro pushed a commit to fmguerreiro/armeria that referenced this issue Sep 19, 2020
Related: line#2404
Motivation:

We allowed Proguard to prune all classes that are not under
`com.linecorp.armeria` (except the internal ones). This can make our own
forked tag interface `reactor.core.scheduler.NonBlocking` to be pruned
by ProGuard, although it is currently kept indirectly and automatically.
It'll be safer if we configure ProGuard explicitly.

Modifications:

- Add ProGuard `keep` for `reactor.core.scheduler.NonBlocking`

Result:

- No chance of regression
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants