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.

Sign up for GitHub

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

Jump to bottom

Replace synchonized block with ReentrantLock to make virtual thread friendly #4510

Open
ikhoon opened this issue Oct 31, 2022 · 13 comments
Open

Replace synchonized block with ReentrantLock to make virtual thread friendly #4510

ikhoon opened this issue Oct 31, 2022 · 13 comments
Labels
improvement

Comments

@ikhoon
Copy link
Contributor

ikhoon commented Oct 31, 2022 •
edited

There is a current known limitation that code should avoid using a synchronized block for the virtual thread.

Related work:
@Bue-von-hon
Copy link
Contributor

hello @ikhoon I would like to work on this issue
Could you please provide a little more information e.g. XXX.class has to be fix.(it seem to be so many file has sync block)
Thank you very much in advance.

@ikhoon
Copy link
Contributor Author

ikhoon commented Nov 23, 2022 •
edited

We need to fix all classes in Armeria that use synchronized blocks.
It is difficult to fix all files at once. Why don't we divide the files into each module?

$ ag --java --ignore-dir gen-src -w -c synchronized 
rxjava3/src/main/java/com/linecorp/armeria/common/rxjava3/RequestContextAssembly.java:2
rxjava2/src/main/java/com/linecorp/armeria/common/rxjava2/RequestContextAssembly.java:2
core/src/main/java/com/linecorp/armeria/internal/server/servlet/ServletTlsAttributes.java:1
core/src/main/java/com/linecorp/armeria/internal/server/annotation/AnnotatedValueResolver.java:1
core/src/main/java/com/linecorp/armeria/internal/server/annotation/AnnotatedBeanFactoryRegistry.java:1
core/src/main/java/com/linecorp/armeria/internal/common/util/MinifiedBouncyCastleProvider.java:2
core/src/main/java/com/linecorp/armeria/internal/common/stream/TwoElementFixedStreamMessage.java:1
core/src/main/java/com/linecorp/armeria/internal/common/ReflectiveDependencyInjector.java:2
core/src/main/java/com/linecorp/armeria/internal/common/metric/CaffeineMetricSupport.java:2
core/src/main/java/com/linecorp/armeria/server/healthcheck/HealthCheckService.java:3
core/src/main/java/com/linecorp/armeria/server/Server.java:6
core/src/main/java/com/linecorp/armeria/common/util/AbstractOption.java:3
core/src/main/java/com/linecorp/armeria/common/util/AbstractListenable.java:3
core/src/main/java/com/linecorp/armeria/common/util/ShutdownHooks.java:1
core/src/main/java/com/linecorp/armeria/common/util/CompositeException.java:2
core/src/main/java/com/linecorp/armeria/common/util/StartStopSupport.java:3
core/src/main/java/com/linecorp/armeria/common/DefaultConcurrentAttributes.java:3
core/src/main/java/com/linecorp/armeria/common/stream/DefaultStreamMessageDuplicator.java:1
core/src/main/java/com/linecorp/armeria/common/stream/DefaultStreamMessage.java:1
core/src/main/java/com/linecorp/armeria/common/DefaultDependencyInjector.java:2
core/src/main/java/com/linecorp/armeria/common/logging/RequestScopedMdc.java:4
core/src/main/java/com/linecorp/armeria/common/logging/DefaultRequestLog.java:2
core/src/main/java/com/linecorp/armeria/client/DefaultEventLoopScheduler.java:1
core/src/main/java/com/linecorp/armeria/client/OneEventLoopState.java:2
core/src/main/java/com/linecorp/armeria/client/HeapBasedEventLoopState.java:2
core/src/main/java/com/linecorp/armeria/client/endpoint/healthcheck/HealthCheckedEndpointGroup.java:6
core/src/main/java/com/linecorp/armeria/client/endpoint/healthcheck/HttpHealthChecker.java:2
core/src/main/java/com/linecorp/armeria/client/endpoint/FileWatcherRegistry.java:3
core/src/main/java/com/linecorp/armeria/client/endpoint/healthcheck/DefaultHealthCheckerContext.java:7
core/src/main/java/com/linecorp/armeria/client/endpoint/WeightedRandomDistributionEndpointSelector.java:1
core/src/main/java/com/linecorp/armeria/client/endpoint/RestartableThread.java:2
tomcat9/src/main/java/com/linecorp/armeria/server/tomcat/JarSubsetResourceSet.java:1
grpc/src/test/java/com/linecorp/armeria/internal/common/grpc/TestServiceImpl.java:7
grpc/src/main/java/com/linecorp/armeria/internal/client/grpc/ArmeriaClientCall.java:1
grpc/src/main/java/com/linecorp/armeria/server/grpc/AbstractServerCall.java:2
grpc/src/main/java/com/linecorp/armeria/server/grpc/GrpcHealthCheckService.java:4
retrofit2/src/main/java/com/linecorp/armeria/client/retrofit2/ArmeriaCallFactory.java:1
retrofit2/src/main/java/com/linecorp/armeria/client/retrofit2/PipeBuffer.java:5
resteasy/src/main/java/com/linecorp/armeria/server/resteasy/ResteasyAsynchronousResponseImpl.java:6
grpc-protocol/src/main/java/com/linecorp/armeria/common/grpc/protocol/AbstractMessageDeframer.java:2
benchmarks/jmh/src/jmh/java/com/linecorp/armeria/common/stream/StreamMessageBenchmark.java:1
reactor3/src/main/java/com/linecorp/armeria/common/reactor3/RequestContextHooks.java:2
spring/boot2-webflux-autoconfigure/src/main/java/com/linecorp/armeria/spring/web/reactive/ArmeriaWebServer.java:2
spring/boot2-autoconfigure/src/main/java/com/linecorp/armeria/spring/AbstractArmeriaBeanPostProcessor.java:2

We may start with common/util and move on to client/endpoint and so on.
In fact, the order does not matter. You can pick anything you want. 😉

@Bue-von-hon
Copy link
Contributor

thank you kindly
then i'll start with common/util until early next week (it has 12 sync block)👍👍

@mscheong01
Copy link
Contributor

I'm also willing to do a part if it's welcome 😄

@Bue-von-hon
Copy link
Contributor

I'm also willing to do a part if it's welcome 😄

glad to work with you!! @j-min5u 😆
i'm working on common/util
What is it like for you to work on client/endpoint if it's ok

@mscheong01
Copy link
Contributor

@Bue-von-hon Sure 😸
I'll get to it 🙌
I'll also see if I could remove the other three files under client/

@mscheong01 mscheong01 mentioned this issue Nov 23, 2022
Merged
@Bue-von-hon Bue-von-hon mentioned this issue Nov 24, 2022
Merged
@ikhoon
Copy link
Contributor Author

ikhoon commented Nov 24, 2022

If all migration has finished, it would be worth prohibiting synchronized block with a checkstyle rule.

@mscheong01
Copy link
Contributor

I'll do internal/ + server/ next
@Bue-von-hon if you do the other files under common/, we'll be done with the core module 😃

@Bue-von-hon
Copy link
Contributor

I'll do internal/ + server/ next
@Bue-von-hon if you do the other files under common/, we'll be done with the core module 😃

ok I'll replace under common/🛠

@mscheong01 mscheong01 mentioned this issue Dec 6, 2022
Merged
minwoox pushed a commit that referenced this issue Dec 9, 2022
@mscheong01
Replace synchronized under core/client with ReentrantLock (#4549)
8b48d0c 
Motivation:

synchronized should be removed because they should be avoided when using virtual threads
resolves part of #4510

Modifications:

- Replaced all _synchronized_  under core/client package with ReentrantLock
minwoox pushed a commit that referenced this issue Dec 14, 2022
@Bue-von-hon
Replace synchronized under common/util with reentrantLock (#4551)
f937294 
Motivation:

for virtual threading, we should replace synchronized with reentrantLock
resolves part of #4510

Modifications:

- replace all synchronized with reetrantLock under `common/util`
Bue-von-hon added a commit to Bue-von-hon/armeria that referenced this issue Dec 17, 2022
@Bue-von-hon
Replace synchronized under core/common with reentrantLock
43c689b 
Motivation:

for virtual threading, we should replace synchronized with reentrantLock
resolves part of line#4510

Modifications:

replace all synchronized with reentrantLock under core/common
@Bue-von-hon Bue-von-hon mentioned this issue Dec 17, 2022
Merged
@daniel-itunu
Copy link
Contributor

daniel-itunu commented Jan 2, 2023 •
edited

@Bue-von-hon @j-min5u joining you guys here. what other module are yet to be done?

@Bue-von-hon
Copy link
Contributor

@Bue-von-hon @j-min5u joining you guys here. what other module are yet to be done?

@daniel-itunu I'm working on a common module right now.🙂

@daniel-itunu
Copy link
Contributor

@Bue-von-hon @j-min5u joining you guys here. what other module are yet to be done?

@daniel-itunu I'm working on a common module right now.🙂
Nice, thanks. taking a look.

@mscheong01
Copy link
Contributor

@daniel-itunu we're currently working on the core module. you could work on other modules like rxjava2/, rxjava3/, grpc/, retrofit2/, etc

@daniel-itunu daniel-itunu mentioned this issue Jan 2, 2023
Closed
@ikhoon ikhoon mentioned this issue Feb 1, 2023
Open
minwoox pushed a commit that referenced this issue Mar 28, 2023
@mscheong01
Replace synchronized under core/internal, core/server with ReentrantL…
6f0a1f6 
…ock (#4562)

Motivation:

synchronized should be removed because they should be avoided when using virtual threads
resolves part of #4510

Modifications:

Replaced all synchronized under internal and server package in core module with ReentrantLock
ikhoon pushed a commit that referenced this issue Jun 3, 2023
@Bue-von-hon @trustin
Replace synchronized under core/common with reentrantLock (#4578)
4c9dccf 
Motivation:
for virtual threading, we should replace synchronized with reentrantLock resolves part of #4510

Modifications:
replace all synchronized with reentrantLock under `core/common`

Result:
no more synchronized blocks on `core/common`

Co-authored-by: Trustin Lee <trustin@linecorp.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
improvement
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
4 participants
@ikhoon @daniel-itunu @Bue-von-hon @mscheong01