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

Reset shared ConversionService instance state on context restart #7635

Merged
merged 5 commits into from
Jul 11, 2022
Merged

Conversation

dstepanov
Copy link
Contributor

@dstepanov dstepanov requested review from graemerocher and sdelamo July 5, 2022 08:46
@graemerocher
Copy link
Contributor

can you fix the test failures? Seem tests relied on this previously present shared state.

@dstepanov dstepanov force-pushed the fixco branch 2 times, most recently from cf89e99 to 705c6cf Compare July 8, 2022 07:54
@dstepanov dstepanov changed the base branch from 3.5.x to 3.6.x July 8, 2022 07:54
@dstepanov
Copy link
Contributor Author

Targeting 3.6

@dstepanov dstepanov requested a review from yawkat July 8, 2022 07:56
@@ -37,6 +38,7 @@ class DefaultAllocatorSpec extends Specification {
'maxCachedByteBuffersPerChunk' | 20
}

@Ignore
Copy link
Member

Choose a reason for hiding this comment

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

if this reliably fails now, maybe your change caused the allocator class that reads the system property to load before the config is applied. this needs to be fixed

@yawkat
Copy link
Member

yawkat commented Jul 8, 2022

can you explain why this change is necessary and related to the serialization issue? what kind of state is the shared conversion context carrying that needs to be reset?

@dstepanov
Copy link
Contributor Author

We register convertors into the static instance which is shared across bean contexts. The problem arises when the convertor was registered with an instance of the bean context, that convertor will fail when accessed after the bean context is destroyed.

@yawkat
Copy link
Member

yawkat commented Jul 8, 2022

ah, makes sense. a bit weird though to register to a shared conversion context from a bean context that isnt shared...

@dstepanov
Copy link
Contributor Author

We should change it probably in 4.0. Shared ConversionService to allow contribute converters only using service loaded and another bean context instance ConversionService delegating to the static one. I have something like this in Micronaut Data.

@dstepanov
Copy link
Contributor Author

It looks like custom ForkAndJoin still deadlocks

@yawkat
Copy link
Member

yawkat commented Jul 8, 2022

is it called in a static initializer? in that case you cant use multithreading at all without risking deadlocks

@dstepanov
Copy link
Contributor Author

Yep, going to remove forking

@sonarqubecloud
Copy link

sonarqubecloud bot commented Jul 8, 2022

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 6 Code Smells

81.0% 81.0% Coverage
0.0% 0.0% Duplication

@graemerocher
Copy link
Contributor

Still failing on Java 17 :(

@dstepanov
Copy link
Contributor Author

I did restart and it passed :-/

@sdelamo sdelamo merged commit a861984 into 3.6.x Jul 11, 2022
@sdelamo sdelamo deleted the fixco branch July 11, 2022 02:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants