armeria icon indicating copy to clipboard operation
armeria copied to clipboard

Fix blocking annotation not working on gRPC calls properly

Open Dogacel opened this issue 1 year ago • 5 comments

Motivation:

#5295

Modifications:

  • Add an explicit property on ctx to 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.

Dogacel avatar Jan 21 '24 04:01 Dogacel

🔍 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

github-actions[bot] avatar Jan 21 '24 05:01 github-actions[bot]

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.

trustin avatar Apr 09 '24 09:04 trustin

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.

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?

Dogacel avatar Jun 08 '24 05:06 Dogacel

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:

trustin avatar Jun 08 '24 16:06 trustin

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.

Dogacel avatar Jun 09 '24 04:06 Dogacel

Bump

Dogacel avatar Jul 29 '24 14:07 Dogacel

Checked CI results:

  • 3 of them failed by flaky tests.
  • The leak will be fixed by #5858

ikhoon avatar Aug 08 '24 06:08 ikhoon