Enhance constructor invocation handling and matching logic#261
Enhance constructor invocation handling and matching logic#261twisti-dev wants to merge 1 commit intoversion/1.21.11from
Conversation
There was a problem hiding this comment.
Pull request overview
This PR enhances the core server’s Java InvocationHandler for reflection proxies by improving how constructors are resolved and invoked, allowing selection of a “best match” constructor when an exact declared-parameter match isn’t available.
Changes:
- Replaced direct constructor
MethodHandlecreation with a newConstructorInvokablethat resolves and invokes constructors at call time. - Implemented a constructor matching/scoring algorithm (including primitive wrapping) plus runtime-type formatting for error messages.
- Bumped project version from
1.21.11-2.70.2to1.21.11-2.70.3.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| surf-api-core/surf-api-core-server/src/main/java/dev/slne/surf/surfapi/core/server/impl/reflection/SurfInvocationHandlerJava.java | Adds constructor invokable + best-match constructor resolution/scoring utilities. |
| gradle.properties | Version bump to 1.21.11-2.70.3. |
| @Override | ||
| public @Nullable Object invoke(final Object[] args) throws Throwable { | ||
| final Constructor<?> constructor = findConstructor(proxiedClass, method, args); | ||
| final MethodHandle handle = normalizeMethodHandleType( | ||
| privateLookup.unreflectConstructor(constructor) | ||
| ); | ||
|
|
There was a problem hiding this comment.
ConstructorInvokable.invoke resolves the constructor, unreflects it, and normalizes the method handle on every call. Compared to the previous approach (creating the handle once in createInvokable), this is a noticeable performance regression for hot paths. Consider caching the selected MethodHandle (at least for the exact declared signature, and optionally per runtime-arg-type signature) inside ConstructorInvokable to avoid repeated reflective scans and handle creation.
| @Override | |
| public @Nullable Object invoke(final Object[] args) throws Throwable { | |
| final Constructor<?> constructor = findConstructor(proxiedClass, method, args); | |
| final MethodHandle handle = normalizeMethodHandleType( | |
| privateLookup.unreflectConstructor(constructor) | |
| ); | |
| /** | |
| * Cached method handle for the last-resolved constructor signature. | |
| * This avoids repeated reflective scans and handle creation on hot paths. | |
| */ | |
| private volatile MethodHandle cachedHandle; | |
| /** | |
| * Runtime parameter types corresponding to {@link #cachedHandle}. | |
| */ | |
| private volatile Class<?>[] cachedParamTypes; | |
| @Override | |
| public @Nullable Object invoke(final Object[] args) throws Throwable { | |
| // Determine the current runtime argument types (nulls are represented as Object.class) | |
| final Class<?>[] currentTypes; | |
| if (args.length == 0) { | |
| currentTypes = new Class<?>[0]; | |
| } else { | |
| currentTypes = new Class<?>[args.length]; | |
| for (int i = 0; i < args.length; i++) { | |
| final Object arg = args[i]; | |
| currentTypes[i] = (arg != null) ? arg.getClass() : Object.class; | |
| } | |
| } | |
| MethodHandle handle = cachedHandle; | |
| final Class<?>[] cachedTypes = cachedParamTypes; | |
| // Recompute the handle if we have no cache yet or the runtime signature differs | |
| if (handle == null || cachedTypes == null || !Arrays.equals(cachedTypes, currentTypes)) { | |
| final Constructor<?> constructor = findConstructor(proxiedClass, method, args); | |
| handle = normalizeMethodHandleType( | |
| privateLookup.unreflectConstructor(constructor) | |
| ); | |
| // Publish the new cache (benign races between threads are acceptable) | |
| cachedHandle = handle; | |
| cachedParamTypes = currentTypes; | |
| } |
| final int score = scoreExecutableMatch(ctorTypes, declaredTypes, args); | ||
| if (score > bestScore) { | ||
| best = constructor; | ||
| bestScore = score; | ||
| } |
There was a problem hiding this comment.
Constructor selection uses a score, but ties (equal score) currently fall back to the first constructor returned by reflection. Since reflection order isn’t specified, this can make constructor choice nondeterministic/unstable when multiple overloads are equally compatible (e.g., multiple interface-typed params that the runtime arg implements). Consider adding a deterministic tie-breaker (e.g., prefer the most specific parameter types) or detect ties and throw an explicit ambiguity error requiring a more specific proxy method signature.
| ); | ||
| } | ||
|
|
||
| best.setAccessible(true); |
There was a problem hiding this comment.
best.setAccessible(true) can throw InaccessibleObjectException under strong encapsulation, and it’s not clear it’s needed here since invocation uses a privateLookup + unreflectConstructor. Prefer trySetAccessible() (and handle failure) or remove the reflective accessibility change if Lookup access is sufficient.
| best.setAccessible(true); | |
| best.trySetAccessible(); |
This pull request introduces improvements to the Java reflection-based invocation handler in the server core, specifically enhancing how constructors are resolved and invoked. The changes focus on making constructor selection more robust and flexible, especially in cases where there may not be an exact parameter type match, and refactor the internal structure to support these enhancements.
Constructor resolution and invocation improvements:
ConstructorInvokableimplementation to handle invoking constructors, replacing the previous direct method handle approach. This allows for more sophisticated constructor selection logic. [1] [2] [3]findConstructormethod to select the best matching constructor based on declared parameter types and runtime argument types, including a scoring system to handle type compatibility and primitive wrapping. This improves reliability when exact matches are not available.scoreExecutableMatch), wrapping primitive types (wrap), and formatting runtime argument types for error messages (runtimeTypesToString).Version update:
gradle.propertiesfrom1.21.11-2.70.2to1.21.11-2.70.3.