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

Some annotated service methods are run from blocking task executors #2187

Merged
merged 32 commits into from Oct 24, 2019

Conversation

@heowc
Copy link
Contributor

heowc commented Oct 14, 2019

Motivation:
The annotated services whose return type is neither HttpResponse nor CompeltableFuture, are run using a blockingTaskExecutor by default.
We should fix this to use EventLoop by default and let a user choose to use blockingTaskExecutor if he/she wants.

Modifications:

  • Add @Blocking which makes the annotated service run using blockingTaskExecutor
  • Make all annotated services run from EventLoop

Result:

  • Fix #2078
  • (Breaking) All annotated services are now run from EventLoop by default
@minwoox minwoox added this to the 0.95.0 milestone Oct 15, 2019
@heowc

This comment has been minimized.

Copy link
Contributor Author

heowc commented Oct 15, 2019

Thank you for reviewing draft!
It will help a lot in further development 👍

@codecov

This comment has been minimized.

Copy link

codecov bot commented Oct 15, 2019

Codecov Report

Merging #2187 into master will increase coverage by 0.02%.
The diff coverage is 93.33%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master    #2187      +/-   ##
============================================
+ Coverage     73.66%   73.69%   +0.02%     
- Complexity     9610     9624      +14     
============================================
  Files           839      839              
  Lines         37000    37020      +20     
  Branches       4557     4560       +3     
============================================
+ Hits          27256    27281      +25     
+ Misses         7418     7415       -3     
+ Partials       2326     2324       -2
Impacted Files Coverage Δ Complexity Δ
...ternal/annotation/AnnotatedHttpServiceFactory.java 87.27% <100%> (+0.03%) 90 <0> (ø) ⬇️
...eria/internal/annotation/AnnotatedHttpService.java 84.79% <93.1%> (+0.44%) 51 <12> (+7) ⬆️
...necorp/armeria/server/GracefulShutdownSupport.java 97.43% <0%> (-2.57%) 4% <0%> (ø)
...a/com/linecorp/armeria/client/HttpChannelPool.java 72.76% <0%> (-2.34%) 59% <0%> (-2%)
...inecorp/armeria/server/HttpResponseSubscriber.java 85.47% <0%> (+1.28%) 75% <0%> (+2%) ⬆️
...rp/armeria/common/stream/DefaultStreamMessage.java 90.78% <0%> (+1.41%) 54% <0%> (+1%) ⬆️
...a/common/grpc/protocol/ArmeriaMessageDeframer.java 72.81% <0%> (+1.94%) 49% <0%> (+3%) ⬆️
.../armeria/client/endpoint/dns/DnsEndpointGroup.java 86.31% <0%> (+2.1%) 15% <0%> (+1%) ⬆️
...om/linecorp/armeria/client/HttpSessionHandler.java 72.03% <0%> (+2.54%) 33% <0%> (+2%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e8c424a...4001a04. Read the comment docs.

@heowc heowc marked this pull request as ready for review Oct 15, 2019
@heowc heowc requested review from ikhoon and trustin as code owners Oct 15, 2019
@heowc heowc changed the title [WIP] Some annotated service methods are run from blocking task executors Some annotated service methods are run from blocking task executors Oct 15, 2019
@heowc

This comment has been minimized.

Copy link
Contributor Author

heowc commented Oct 17, 2019

I was noticed that if only use event loop thread, run into problems when block for requests on the server.
(I think the cause of the failure of CI is the same as this)

The problem code in the test is:

@Post
@Path("/a/string-aggregate-response2")
public AggregatedHttpResponse postStringAggregateResponse2(
    HttpRequest req, RequestContext ctx) {
  validateContextAndRequest(ctx, req);
  final AggregatedHttpRequest request = req.aggregate().join();
  return AggregatedHttpResponse.of(ResponseHeaders.of(HttpStatus.OK), request.content());
}

I'm going to look a bit further to solve the problem. Or should we use @Blocking for this test?

@minwoox

This comment has been minimized.

Copy link
Member

minwoox commented Oct 18, 2019

I'm going to look a bit further to solve the problem. Or should we use @Blocking for this test?

Let's get rid of the test. 😄

@heowc

This comment has been minimized.

Copy link
Contributor Author

heowc commented Oct 18, 2019

I'm going to look a bit further to solve the problem. Or should we use @Blocking for this test?

Let's get rid of the test. 😄

Oh. Is that okay?
If there is similar test code again, this should also be removed.

@minwoox

This comment has been minimized.

Copy link
Member

minwoox commented Oct 18, 2019

Oh. Is that okay?
If there is similar test code again, this should also be removed.

I think so. 😄 That's the reason why we needed the migration plan, but we have decided to just make a breaking change.

@heowc heowc requested review from minwoox, trustin and ikhoon Oct 18, 2019
Copy link
Member

trustin left a comment

Excellent job, @heowc! 👍

@ikhoon
ikhoon approved these changes Oct 19, 2019
class AnnotatedHttpServiceTest {

@ClassRule
public static final ServerRule rule = new ServerRule() {
@RegisterExtension
static final ServerExtension server = new ServerExtension() {
Comment on lines +91 to +94

This comment has been minimized.

Copy link
@ikhoon

ikhoon Oct 19, 2019

Contributor

👍

@ikhoon

This comment has been minimized.

Copy link
Contributor

ikhoon commented Oct 19, 2019

Nice work! 👍

Copy link
Member

minwoox left a comment

Just one nit. Thanks a lot, @heowc!

@minwoox

This comment has been minimized.

Copy link
Member

minwoox commented Oct 23, 2019

@heowc Oops, could you resolve the conflict, please?

heowc added 3 commits Oct 23, 2019
…ation

# Conflicts:
#	core/src/main/java/com/linecorp/armeria/internal/annotation/AnnotatedHttpServiceFactory.java
@heowc

This comment has been minimized.

Copy link
Contributor Author

heowc commented Oct 23, 2019

@minwoox I have resolved the conflict!

@minwoox minwoox merged commit 8974da9 into line:master Oct 24, 2019
4 checks passed
4 checks passed
codecov/patch 93.33% of diff hit (target 73.66%)
Details
codecov/project 73.69% (+0.02%) compared to e8c424a
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
license/cla Contributor License Agreement is signed.
Details
@minwoox

This comment has been minimized.

Copy link
Member

minwoox commented Oct 24, 2019

Thanks, @heowc! 👍

@heowc heowc deleted the heowc:add_blocking_annotation branch Oct 24, 2019
sivaalli added a commit to sivaalli/armeria that referenced this pull request Oct 30, 2019
…e#2187)

Motivation:
The annotated services whose return type is neither `HttpResponse` nor `CompeltableFuture`, are run using a `blockingTaskExecutor` by default.
We should fix this to use `EventLoop` by default and let a user choose to use `blockingTaskExecutor` if he/she wants.

Modifications:
- Add `@Blocking` which makes the annotated service run using `blockingTaskExecutor`
- Make all annotated services run from `EventLoop`

Result:
- Fix line#2078
- (Breaking) All annotated services are now run from `EventLoop` by default
eugene70 added a commit to eugene70/armeria that referenced this pull request Nov 10, 2019
…e#2187)

Motivation:
The annotated services whose return type is neither `HttpResponse` nor `CompeltableFuture`, are run using a `blockingTaskExecutor` by default.
We should fix this to use `EventLoop` by default and let a user choose to use `blockingTaskExecutor` if he/she wants.

Modifications:
- Add `@Blocking` which makes the annotated service run using `blockingTaskExecutor`
- Make all annotated services run from `EventLoop`

Result:
- Fix line#2078
- (Breaking) All annotated services are now run from `EventLoop` by default
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.