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

Fix blocking annotation not working on gRPC calls properly #5399

Conversation

Dogacel
Copy link
Contributor

@Dogacel Dogacel commented Jan 21, 2024

Motivation:

#5295

Modifications:

  • Add an explicit property on ctx to determine whether the blocking task executor should be used or not.

Result:

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.

@github-actions github-actions bot added the Stale label Apr 9, 2024
@trustin
Copy link
Member

trustin commented Apr 9, 2024

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.

@github-actions github-actions bot removed the Stale label Apr 10, 2024
@github-actions github-actions bot added the Stale label May 10, 2024
@Dogacel
Copy link
Contributor Author

Dogacel commented Jun 8, 2024

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?

@trustin
Copy link
Member

trustin commented Jun 8, 2024

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. 😅

@github-actions github-actions bot removed the Stale label Jun 9, 2024
@Dogacel
Copy link
Contributor Author

Dogacel commented Jun 9, 2024

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 Dogacel force-pushed the dogac/fix-blocking-annotation-in-blocking-task-executor branch from 32e4b01 to b68335e Compare June 9, 2024 04:59
@github-actions github-actions bot added the Stale label Jul 10, 2024
@Dogacel
Copy link
Contributor Author

Dogacel commented Jul 29, 2024

Bump

@github-actions github-actions bot removed the Stale label Jul 30, 2024
Copy link
Contributor

@ikhoon ikhoon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry for the late review. 😅 Left a comment for the direction.

@ikhoon ikhoon added the defect label Jul 31, 2024
@ikhoon ikhoon modified the milestones: 1.30.0, 1.31.0 Jul 31, 2024
@Dogacel Dogacel force-pushed the dogac/fix-blocking-annotation-in-blocking-task-executor branch from bd14f7f to 96ea44f Compare August 6, 2024 22:02
Copy link
Contributor

@ikhoon ikhoon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, @Dogacel! 👍👍

@ikhoon ikhoon removed this from the 1.31.0 milestone Aug 7, 2024
@ikhoon ikhoon added this to the 1.30.0 milestone Aug 7, 2024
Copy link
Contributor

@minwoox minwoox left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, @Dogacel! 👍

Copy link
Contributor

@jrhee17 jrhee17 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 👍 👍

@ikhoon
Copy link
Contributor

ikhoon commented Aug 8, 2024

Checked CI results:

@ikhoon ikhoon merged commit cdfad4f into line:main Aug 8, 2024
11 of 15 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Using @Blocking annotation for grpc+kotlin doesn't change the backing thread of coroutines
5 participants