Fix blocking annotation not working on gRPC calls properly
Motivation:
#5295
Modifications:
- Add an explicit property on
ctxto determine whether the blocking task executor should be used or not.
Result:
- Closes #5295
Maybe we could have just checked whether the blockingTaskExecutor is null or not but I did not like that idea. Also this is the recommended way in the issue description.
Will add tests soon.
🔍 Build Scan® (commit: d458ec4bc42400db37f0ff17b6805a79efe2eecd)
| Job name | Status | Build Scan® |
|---|---|---|
| build-self-hosted-unsafe-jdk-8 | ✅ | https://ge.armeria.dev/s/ghkneexuz56qe |
| build-self-hosted-unsafe-jdk-21-snapshot-blockhound | ❌ (failure) | https://ge.armeria.dev/s/rn5ygxteecvvo |
| build-self-hosted-unsafe-jdk-17-min-java-17-coverage | ✅ | https://ge.armeria.dev/s/qnj47ptboc5ng |
| build-self-hosted-unsafe-jdk-17-min-java-11 | ✅ | https://ge.armeria.dev/s/y2so6uqgmbuq4 |
| build-self-hosted-unsafe-jdk-17-leak | ❌ (failure) | https://ge.armeria.dev/s/vdcoy5z26clf6 |
| build-self-hosted-unsafe-jdk-11 | ✅ | https://ge.armeria.dev/s/sgqven4bsl2ta |
| build-macos-12-jdk-21 | ❌ (failure) | https://ge.armeria.dev/s/syvwdy6btucwu |
The current plan is to deprecate ctx.eventLoop and provide a way for a user to specify an arbitrary executor or even a virtual thread executor via ServiceConfig.serviceWorkerGroup, which will eventually make blockingTaskExecutor obsolete.
The current plan is to deprecate
ctx.eventLoopand provide a way for a user to specify an arbitrary executor or even a virtual thread executor viaServiceConfig.serviceWorkerGroup, which will eventually makeblockingTaskExecutorobsolete.
Do you prefer to close this PR and continue with the long term approach or resolve this issue first and iteratively move towards the ultimate goal of removing blocking executor?
Both are fine by me. Let me know if you need this PR to be merged, we'd be happy to accept it given it will take quite some time until we fix this properly. :sweat_smile:
Both are fine by me. Let me know if you need this PR to be merged, we'd be happy to accept it given it will take quite some time until we fix this properly. 😅
I think this is a pretty important issue for teams with less experience. Previously, @Blocking was being ignored and service was still using the event loop. We have faced performance issues regarding blocking calls so this bug might exist in many people's codebase and secretly eat out some performance. We might be affected by this as well!
I have went ahead and added tests to verify this fix works PTAL.
Bump
Checked CI results:
- 3 of them failed by flaky tests.
- The leak will be fixed by #5858