-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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: hessian deserialize support sofa.serialize.dynamic.load.enable #1463
base: master
Are you sure you want to change the base?
Conversation
WalkthroughThe changes modify the Changes
Poem
Tip CodeRabbit's docstrings feature is now available as part of our Early Access Program! Simply use the command Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (2)
codec/codec-sofa-hessian/src/main/java/com/alipay/sofa/rpc/codec/sofahessian/SingleClassLoaderSofaSerializerFactory.java (2)
52-55
: Consider improving configurability & debugging.
ReadingdynamicLoadEnable
from a system property is straightforward. For easier maintenance, you might log whether dynamic loading is enabled at startup, or allow overrides via environment variables or a config file.
100-112
: Refactor synchronization to usecomputeIfAbsent
.
In Java 8 and above, you can simplify the synchronization block by usingConcurrentHashMap.computeIfAbsent
. It avoids explicit locking and ensures thread-safe creation of new sub-maps:- Map<String, Object> typeMap = _typeNotFoundMap.get(appClassLoader); - if (typeMap == null) { - synchronized (this) { - typeMap = _typeNotFoundMap.get(appClassLoader); - if (typeMap == null) { - _typeNotFoundMap.put(appClassLoader, new ConcurrentHashMap<String, Object>(8)); - typeMap = _typeNotFoundMap.get(appClassLoader); - } - } - } - typeMap.put(type, NOT_FOUND); + Map<String, Object> typeMap = _typeNotFoundMap.computeIfAbsent( + appClassLoader, cl -> new ConcurrentHashMap<>(8)); + typeMap.put(type, NOT_FOUND);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
codec/codec-sofa-hessian/src/main/java/com/alipay/sofa/rpc/codec/sofahessian/SingleClassLoaderSofaSerializerFactory.java
(3 hunks)
🔇 Additional comments (4)
codec/codec-sofa-hessian/src/main/java/com/alipay/sofa/rpc/codec/sofahessian/SingleClassLoaderSofaSerializerFactory.java (4)
30-31
: Imports are well-chosen.
Bringing in Map
and ConcurrentHashMap
is correct and lays the foundation for a thread-safe caching mechanism.
49-50
: Potential unbounded growth.
Be mindful that _typeNotFoundMap
may grow unbounded if many non-existent types are attempted. Consider an explicit strategy for cache invalidation or size-limiting if necessary.
51-51
: Appropriate use of a sentinel object.
Using NOT_FOUND
as a marker is a clean approach, but ensure future expansions don't require additional metadata for missing types.
88-95
: Short-circuit usage when dynamic loading is disabled.
Your logic correctly avoids repeated failed lookups by consulting _typeNotFoundMap
. This is an efficient approach for reducing overhead.
.../main/java/com/alipay/sofa/rpc/codec/sofahessian/SingleClassLoaderSofaSerializerFactory.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (1)
codec/codec-sofa-hessian/src/main/java/com/alipay/sofa/rpc/codec/sofahessian/SingleClassLoaderSofaSerializerFactory.java (1)
49-54
: LGTM! Thread-safe implementation of type caching.The implementation uses appropriate concurrent collections and follows the existing naming conventions. The system property control provides the necessary configuration flexibility.
Note: Consider documenting the system property key
DYNAMIC_LOAD_ENABLE_KEY
in the class-level JavaDoc to make it more discoverable for users.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
codec/codec-sofa-hessian/src/main/java/com/alipay/sofa/rpc/codec/sofahessian/SingleClassLoaderSofaSerializerFactory.java
(3 hunks)
🔇 Additional comments (3)
codec/codec-sofa-hessian/src/main/java/com/alipay/sofa/rpc/codec/sofahessian/SingleClassLoaderSofaSerializerFactory.java (3)
88-95
: LGTM! Efficient early-return optimization for known missing types.
The implementation correctly checks the cache before attempting class loading, providing an optimization for scenarios where types are known to be unavailable.
100-102
: LGTM! Thread-safe caching of not found types.
The implementation correctly uses computeIfAbsent
for thread-safe updates to the cache when a ClassNotFoundException occurs. This aligns with the PR objective of caching class loading results.
Note: This addresses the previous review comment by @OrezzerO about using computeIfAbsent
.
52-54
: Verify consistent system property usage across the codebase.
Let's ensure the system property DYNAMIC_LOAD_ENABLE_KEY
is consistently used and documented across the codebase.
✅ Verification successful
System property usage is properly scoped and implemented
The verification shows that:
- The
DYNAMIC_LOAD_ENABLE_KEY
is only used withinSingleClassLoaderSofaSerializerFactory
class - The property controls class loading optimization by caching "not found" classes when dynamic loading is disabled
- The implementation is thread-safe using
ConcurrentHashMap
- The default value is properly set to
Boolean.FALSE.toString()
The system property usage is consistent and well-contained within its specific use case of controlling dynamic class loading behavior in the Hessian serializer factory.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other usages of the DYNAMIC_LOAD_ENABLE_KEY constant
rg -l "DYNAMIC_LOAD_ENABLE_KEY"
# Search for direct string references to the property
rg -l "sofa\.serialize\.dynamic\.load\.enable"
# Look for documentation of the property
fd README.md | xargs rg "sofa\.serialize\.dynamic\.load\.enable"
Length of output: 268
Script:
#!/bin/bash
# Let's examine the actual implementation and usage of DYNAMIC_LOAD_ENABLE_KEY
ast-grep --pattern 'class $_ {
$$$
DYNAMIC_LOAD_ENABLE_KEY
$$$
}'
# Let's also check the content of the file to understand the constant definition
cat codec/codec-sofa-hessian/src/main/java/com/alipay/sofa/rpc/codec/sofahessian/SingleClassLoaderSofaSerializerFactory.java
# Check for any configuration or property-related files
fd -e properties -e yaml -e yml | xargs rg "sofa\.serialize\.dynamic\.load\.enable"
Length of output: 6285
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (1)
codec/codec-sofa-hessian/src/test/java/com/alipay/sofa/rpc/codec/sofahessian/SingleClassLoaderSofaSerializerFactoryTest.java (1)
70-80
: Reflection usage for internal state testing.Accessing the private
_typeNotFoundMap
field validates genuine behavior but introduces potential brittleness if the field’s name or access level changes in future refactors. Consider providing a public getter for test-only or a dedicated test utility to reduce reflection reliance. Otherwise, the logic verifying_typeNotFoundMap
size changes is correct.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
codec/codec-sofa-hessian/src/test/java/com/alipay/sofa/rpc/codec/sofahessian/SingleClassLoaderSofaSerializerFactoryTest.java
(2 hunks)
🔇 Additional comments (2)
codec/codec-sofa-hessian/src/test/java/com/alipay/sofa/rpc/codec/sofahessian/SingleClassLoaderSofaSerializerFactoryTest.java (2)
19-19
: Imports are consistent and necessary.
The newly added imports for Deserializer
, SerializerFactory
, Assert
, Field
, and various utilities are correct and do not introduce conflicts. Good to see that each import is strictly required for the added tests.
Also applies to: 22-22, 29-29
82-98
: System property manipulation is well-handled.
Toggling the dynamic loading behavior via System.setProperty
and reverting it in a finally
block is a good practice. This ensures the test environment remains clean. The checks on _typeNotFoundMap
again validate the internal caching mechanism accurately.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (4)
codec/codec-sofa-hessian/src/main/java/com/alipay/sofa/rpc/codec/sofahessian/SingleClassLoaderSofaSerializerFactory.java (2)
49-54
: LGTM! Consider renaming constant to follow Java naming convention.The field declarations look good. The use of ConcurrentHashMap is appropriate for thread-safe caching of not found types.
Consider renaming
NOT_FOUND
toTYPE_NOT_FOUND
to better reflect its purpose and follow Java constant naming conventions.
88-95
: Simplify nested conditions for better readability.The logic is correct but can be simplified to reduce nesting.
Consider this more concise version:
- if (!dynamicLoadEnable) { - Map<String, Object> typeMap = _typeNotFoundMap.get(appClassLoader); - if (typeMap != null) { - if (typeMap.containsKey(type)) { - return null; - } - } - } + if (!dynamicLoadEnable && + _typeNotFoundMap.containsKey(appClassLoader) && + _typeNotFoundMap.get(appClassLoader).containsKey(type)) { + return null; + }codec/codec-sofa-hessian/src/test/java/com/alipay/sofa/rpc/codec/sofahessian/SingleClassLoaderSofaSerializerFactoryTest.java (2)
70-81
: LGTM! Consider using BDD-style test naming.The test effectively verifies the caching behavior when dynamic loading is disabled.
Consider renaming the test method to better describe the behavior being tested:
- public void testDynamicLoadDisabled() throws Exception { + public void shouldCacheNotFoundTypesWhenDynamicLoadingIsDisabled() throws Exception {
83-99
: LGTM! Consider improving system property handling.The test effectively verifies that type not found caching is disabled when dynamic loading is enabled.
Consider these improvements:
- Rename the test method for better clarity:
- public void testDynamicLoadEnabled() throws Exception { + public void shouldNotCacheNotFoundTypesWhenDynamicLoadingIsEnabled() throws Exception {
- Use a helper method or JUnit rule to manage system properties:
private void withSystemProperty(String key, String value, Runnable test) { String original = System.getProperty(key); try { System.setProperty(key, value); test.run(); } finally { if (original == null) { System.clearProperty(key); } else { System.setProperty(key, original); } } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
codec/codec-sofa-hessian/src/main/java/com/alipay/sofa/rpc/codec/sofahessian/SingleClassLoaderSofaSerializerFactory.java
(3 hunks)codec/codec-sofa-hessian/src/test/java/com/alipay/sofa/rpc/codec/sofahessian/SingleClassLoaderSofaSerializerFactoryTest.java
(2 hunks)
🔇 Additional comments (1)
codec/codec-sofa-hessian/src/main/java/com/alipay/sofa/rpc/codec/sofahessian/SingleClassLoaderSofaSerializerFactory.java (1)
100-102
: LGTM! Efficient implementation using computeIfAbsent.
The error handling and caching implementation looks good. The use of computeIfAbsent provides thread-safe initialization of the inner map.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Motivation:
In sofa-hessian, there is a switch called sofa.serialize.dynamic.load.enable. When set to true, if a class is not found during deserialization, the result will be recorded in the internal cache to avoid repeated class loading operations in the future. In sofa-rpc, due to the rewriting of related classes, this feature is missing.
Modification:
Referring to the implementation of sofa-hessian, support for the sofa.serialize.dynamic.load.enable switch has been added.
Summary by CodeRabbit