Copilot commented on code in PR #13095:
URL: https://github.com/apache/ignite/pull/13095#discussion_r3433268467
##########
modules/core/src/main/java/org/apache/ignite/internal/managers/communication/GridIoManager.java:
##########
@@ -465,6 +466,9 @@ public void resetMetrics() {
msg.getClass().getName() + ". Most likely
GridCommunicationSpi is being used directly, " +
"which is illegal - make sure to send messages
only via GridProjection API.");
}
+ catch (IgniteCheckedException e) {
+ throw new IgniteException(e);
+ }
Review Comment:
`CommunicationListener#onMessage` now wraps `IgniteCheckedException` into
`IgniteException` and rethrows. Since this runs on the communication SPI
thread, rethrowing an unchecked exception can break message processing /
destabilize the node; it’s safer to log and ignore the malformed message
(similar to existing error-handling patterns in this class).
##########
modules/core/src/main/java/org/apache/ignite/internal/processors/cache/IgniteCacheOffheapManagerImpl.java:
##########
@@ -995,13 +995,18 @@ private long allocateForTree() throws
IgniteCheckedException {
assert info.ttl() == TTL_ETERNAL : info.ttl();
- batch.add(new DataRowCacheAware(info.key(),
- info.value(),
- info.version(),
- part.id(),
- info.expireTime(),
- info.cacheId(),
- grp.storeCacheIdInDataPage()));
+ try {
+ batch.add(new DataRowCacheAware(info.key(),
+ info.value(),
+ info.version(),
+ part.id(),
+ info.expireTime(),
+ info.cacheId(),
+ grp.storeCacheIdInDataPage()));
+ }
+ catch (IllegalStateException th) {
+ assert ctx.cacheContext(grp.groupId()) == null; // Ignoring
removed cache entries.
+ }
Review Comment:
The catch block asserts `ctx.cacheContext(info.grp.groupId()) == null`, but
`cacheContext(int)` expects a *cacheId*, not a groupId. This assertion can be
false even when the cache was removed and may hide real issues. Use
`info.cacheId()` here (and the exception variable is unused).
##########
modules/zookeeper/src/main/java/org/apache/ignite/spi/discovery/zk/internal/DiscoveryMessageParser.java:
##########
@@ -126,6 +140,13 @@ private <T extends Message> T
deserializeMessage(InputStream in) throws IOExcept
}
while (!finished);
+ try {
+ msgSer.finishUnmarshal(msg, ((IgniteEx)spi.ignite()).context());
+ }
+ catch (IgniteCheckedException e) {
+ throw new IgniteSpiException("Failed to unmarshal joining node
data", e);
+ }
Review Comment:
Error message mentions "joining node data", but this parser unmarshals
arbitrary ZooKeeper discovery messages. Please update the message to match the
actual operation to aid troubleshooting.
##########
modules/core/src/main/java/org/apache/ignite/internal/processors/cache/GridCacheReturn.java:
##########
@@ -121,7 +123,26 @@ public GridCacheReturn(
/**
* @return Value.
*/
- @Nullable public <V> V value() {
+ @Nullable public <V> V value(GridCacheContext ctx) {
+ if (v == null) {
+ if (cacheObj != null)
+ v = ctx.cacheObjectContext().unwrapBinaryIfNeeded(cacheObj,
true, false, U.gridClassLoader());
+
+ if (invokeRes && invokeResCol != null) {
+ Map<Object, CacheInvokeResult> map0 =
U.newHashMap(invokeResCol.size());
+
+ for (CacheInvokeDirectResult res : invokeResCol) {
+ CacheInvokeResult<?> res0 = res.error() == null ?
+
CacheInvokeResult.fromResult(ctx.cacheObjectContext().unwrapBinaryIfNeeded(res.result(),
true, false, null)) :
+ CacheInvokeResult.fromError(res.error());
+
+
map0.put(ctx.cacheObjectContext().unwrapBinaryIfNeeded(res.key(), true, false,
null), res0);
+ }
+
+ v = map0;
+ }
+ }
+
return (V)v;
}
Review Comment:
`GridCacheReturn.value(GridCacheContext)` unwraps cache objects using
`U.gridClassLoader()`. That ignores the configured/deployment classloader and
can fail to deserialize user types (especially with P2P classloading or a
custom `IgniteConfiguration#setClassLoader`). Use the cache context’s
deployment/global loader consistently for all unwrap operations.
##########
modules/core/src/main/java/org/apache/ignite/internal/processors/cache/GridCacheIoManager.java:
##########
@@ -1551,7 +1552,9 @@ private void unmarshall(UUID nodeId, GridCacheMessage
cacheMsg) {
log.debug("Set P2P context [senderId=" + nodeId + ", msg="
+ cacheMsg + ']');
}
- cacheMsg.finishUnmarshal(cctx, cctx.deploy().globalLoader());
+ MessageSerializer ser =
cctx.kernalContext().messageFactory().serializer(cacheMsg.directType());
+
+ ser.finishUnmarshal(cacheMsg, cctx.kernalContext(), null,
cctx.deploy().globalLoader());
}
Review Comment:
Only `finishUnmarshal(msg, kctx, nested, clsLdr)` is invoked here. The
generated serializers use `finishUnmarshal(msg, kctx)` to complete
unmarshalling for `MarshallableMessage` fields (e.g., `ErrorMessage`), while
the 4-arg overload handles cache-object traversal. To fully initialize
messages, call both overloads (kctx-only first) before processing the message
further.
##########
modules/core/src/main/java/org/apache/ignite/internal/processors/cache/transactions/TxLocksRequest.java:
##########
@@ -89,33 +90,20 @@ public Collection<IgniteTxKey> txKeys() {
}
/** {@inheritDoc} */
- @Override public void prepareMarshal(GridCacheSharedContext<?, ?> ctx)
throws IgniteCheckedException {
- super.prepareMarshal(ctx);
-
+ @Override public void prepareMarshal(Marshaller marsh) throws
IgniteCheckedException {
txKeysArr = new IgniteTxKey[txKeys.size()];
int i = 0;
- for (IgniteTxKey key : txKeys) {
- key.prepareMarshal(ctx.cacheContext(key.cacheId()));
-
+ for (IgniteTxKey key : txKeys)
txKeysArr[i++] = key;
- }
}
/** {@inheritDoc} */
- @Override public void finishUnmarshal(GridCacheSharedContext<?, ?> ctx,
ClassLoader ldr) throws IgniteCheckedException {
- super.finishUnmarshal(ctx, ldr);
-
+ @Override public void finishUnmarshal(Marshaller marsh, ClassLoader
clsLdr) throws IgniteCheckedException {
txKeys = U.newHashSet(txKeysArr.length);
- for (IgniteTxKey key : txKeysArr) {
- key.finishUnmarshal(ctx.cacheContext(key.cacheId()), ldr);
-
+ for (IgniteTxKey key : txKeysArr)
txKeys.add(key);
- }
-
- txKeysArr = null;
}
Review Comment:
`TxLocksRequest.finishUnmarshal(...)` reconstructs `txKeys` from `txKeysArr`
but never clears `txKeysArr`. Keeping the array after unmarshalling increases
retained memory and keeps two equivalent representations alive longer than
needed.
##########
modules/zookeeper/src/main/java/org/apache/ignite/spi/discovery/zk/internal/DiscoveryMessageParser.java:
##########
@@ -85,6 +92,13 @@ private void serializeMessage(Message m, OutputStream out)
throws IOException {
MessageSerializer msgSer = msgFactory.serializer(m.directType());
+ try {
+ msgSer.prepareMarshal(m, ((IgniteEx)spi.ignite()).context(), null);
+ }
+ catch (IgniteCheckedException e) {
+ throw new IgniteSpiException("Failed to marshal joining node
data", e);
+ }
Review Comment:
Error message mentions "joining node data", but this parser marshals
arbitrary ZooKeeper discovery messages. Using an inaccurate message makes
production debugging harder; please make it specific to discovery message
marshalling.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
