anton-vinogradov commented on code in PR #13095: URL: https://github.com/apache/ignite/pull/13095#discussion_r3588946028
########## modules/core/src/main/java/org/apache/ignite/internal/managers/communication/MessageUnmarshalDedup.java: ########## @@ -0,0 +1,112 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.apache.ignite.internal.managers.communication; + +import java.lang.ref.Reference; +import java.lang.ref.ReferenceQueue; +import java.lang.ref.WeakReference; +import java.util.Set; +import java.util.concurrent.ConcurrentHashMap; +import org.apache.ignite.IgniteSystemProperties; +import org.apache.ignite.internal.MarshallableMessage; +import org.apache.ignite.plugin.extensions.communication.Message; +import org.apache.ignite.plugin.extensions.communication.MessageMarshaller; + +/** + * Detects a {@link MarshallableMessage} instance being finish-unmarshalled more than once within the same pass + * (cache-aware or cache-free) — a class-loader or receive-path bug. The two passes over one message (e.g. the + * generic {@code GridIoManager} pass plus a subsystem's cache-aware pass) are legitimate and tracked separately. + * Gated by {@link #ENABLED}, so it runs only under tests and is folded away in production. + * + * @see MessageMarshaller + */ +public class MessageUnmarshalDedup { + /** + * When {@code true}, the no-double-unmarshal check runs. {@code static final} so the JIT folds the guard away + * in production (even with assertions on); enabled only by tests via {@code IGNITE_MESSAGE_UNMARSHAL_ONCE_CHECK}. + */ + public static final boolean ENABLED = IgniteSystemProperties.getBoolean(IgniteSystemProperties.IGNITE_MESSAGE_UNMARSHAL_ONCE_CHECK); + + /** Queue of collected referents, drained on each call to evict stale {@link IdRef}s from {@link #SEEN}. */ + private static final ReferenceQueue<Message> Q = new ReferenceQueue<>(); + + /** Finish-unmarshalled instances, held weakly and keyed by identity so they vanish with the message. */ + private static final Set<IdRef> SEEN = ConcurrentHashMap.newKeySet(); + + /** */ + private MessageUnmarshalDedup() { + // No-op. + } + + /** + * @param msg Message about to be finish-unmarshalled. + * @param cacheMode {@code true} for the cache-aware pass, {@code false} for the cache-free pass; the two passes + * over one message are legitimate and tracked separately, so only a repeat of the same pass is reported. + * @return {@code true} if {@code msg} is not a {@link MarshallableMessage} or is finish-unmarshalled the first + * time in this pass. + */ + public static boolean firstUnmarshal(Message msg, boolean cacheMode) { + if (!(msg instanceof MarshallableMessage)) + return true; + + for (Reference<? extends Message> r; (r = Q.poll()) != null; ) + SEEN.remove(r); + + return SEEN.add(new IdRef(msg, cacheMode)); + } + + /** Weak reference to a message keyed by (identity, pass), so distinct messages and the two passes stay distinct. */ Review Comment: Reworded with `{@code}` — `identityHashCode` folded with `cacheMode`. ########## modules/core/src/main/java/org/apache/ignite/internal/managers/communication/MessageUnmarshalDedup.java: ########## @@ -0,0 +1,112 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.apache.ignite.internal.managers.communication; + +import java.lang.ref.Reference; +import java.lang.ref.ReferenceQueue; +import java.lang.ref.WeakReference; +import java.util.Set; +import java.util.concurrent.ConcurrentHashMap; +import org.apache.ignite.IgniteSystemProperties; +import org.apache.ignite.internal.MarshallableMessage; +import org.apache.ignite.plugin.extensions.communication.Message; +import org.apache.ignite.plugin.extensions.communication.MessageMarshaller; + +/** + * Detects a {@link MarshallableMessage} instance being finish-unmarshalled more than once within the same pass + * (cache-aware or cache-free) — a class-loader or receive-path bug. The two passes over one message (e.g. the + * generic {@code GridIoManager} pass plus a subsystem's cache-aware pass) are legitimate and tracked separately. + * Gated by {@link #ENABLED}, so it runs only under tests and is folded away in production. + * + * @see MessageMarshaller + */ +public class MessageUnmarshalDedup { + /** + * When {@code true}, the no-double-unmarshal check runs. {@code static final} so the JIT folds the guard away + * in production (even with assertions on); enabled only by tests via {@code IGNITE_MESSAGE_UNMARSHAL_ONCE_CHECK}. + */ + public static final boolean ENABLED = IgniteSystemProperties.getBoolean(IgniteSystemProperties.IGNITE_MESSAGE_UNMARSHAL_ONCE_CHECK); + + /** Queue of collected referents, drained on each call to evict stale {@link IdRef}s from {@link #SEEN}. */ + private static final ReferenceQueue<Message> Q = new ReferenceQueue<>(); + + /** Finish-unmarshalled instances, held weakly and keyed by identity so they vanish with the message. */ + private static final Set<IdRef> SEEN = ConcurrentHashMap.newKeySet(); + + /** */ + private MessageUnmarshalDedup() { + // No-op. + } + + /** + * @param msg Message about to be finish-unmarshalled. + * @param cacheMode {@code true} for the cache-aware pass, {@code false} for the cache-free pass; the two passes + * over one message are legitimate and tracked separately, so only a repeat of the same pass is reported. + * @return {@code true} if {@code msg} is not a {@link MarshallableMessage} or is finish-unmarshalled the first + * time in this pass. + */ + public static boolean firstUnmarshal(Message msg, boolean cacheMode) { + if (!(msg instanceof MarshallableMessage)) + return true; + + for (Reference<? extends Message> r; (r = Q.poll()) != null; ) + SEEN.remove(r); + + return SEEN.add(new IdRef(msg, cacheMode)); + } + + /** Weak reference to a message keyed by (identity, pass), so distinct messages and the two passes stay distinct. */ + private static final class IdRef extends WeakReference<Message> { + /** Referent identity hash folded with the pass, captured up front since the referent may be cleared later. */ + private final int hash; + + /** Unmarshal pass: cache-aware vs cache-free. Keeps the two legitimate passes over one message distinct. */ + private final boolean cacheMode; + + /** + * @param msg Tracked message. + * @param cacheMode Unmarshal pass. + */ + IdRef(Message msg, boolean cacheMode) { + super(msg, Q); + + this.cacheMode = cacheMode; + hash = 31 * System.identityHashCode(msg) + (cacheMode ? 1 : 0); + } + + /** {@inheritDoc} */ + @Override public int hashCode() { + return hash; + } + + /** {@inheritDoc} */ + @Override public boolean equals(Object o) { + if (this == o) + return true; + + if (!(o instanceof IdRef)) + return false; + + IdRef ref = (IdRef)o; + + Message m = get(); Review Comment: Yes — `get()` is read once into `m` on purpose: a concurrent GC could clear the referent between the null check and the `==` compare, so we capture it first (added a comment saying so). ########## modules/core/src/main/java/org/apache/ignite/internal/managers/communication/MessageUnmarshalDedup.java: ########## @@ -0,0 +1,112 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.apache.ignite.internal.managers.communication; + +import java.lang.ref.Reference; +import java.lang.ref.ReferenceQueue; +import java.lang.ref.WeakReference; +import java.util.Set; +import java.util.concurrent.ConcurrentHashMap; +import org.apache.ignite.IgniteSystemProperties; +import org.apache.ignite.internal.MarshallableMessage; +import org.apache.ignite.plugin.extensions.communication.Message; +import org.apache.ignite.plugin.extensions.communication.MessageMarshaller; + +/** + * Detects a {@link MarshallableMessage} instance being finish-unmarshalled more than once within the same pass + * (cache-aware or cache-free) — a class-loader or receive-path bug. The two passes over one message (e.g. the + * generic {@code GridIoManager} pass plus a subsystem's cache-aware pass) are legitimate and tracked separately. + * Gated by {@link #ENABLED}, so it runs only under tests and is folded away in production. + * + * @see MessageMarshaller + */ +public class MessageUnmarshalDedup { + /** + * When {@code true}, the no-double-unmarshal check runs. {@code static final} so the JIT folds the guard away + * in production (even with assertions on); enabled only by tests via {@code IGNITE_MESSAGE_UNMARSHAL_ONCE_CHECK}. + */ + public static final boolean ENABLED = IgniteSystemProperties.getBoolean(IgniteSystemProperties.IGNITE_MESSAGE_UNMARSHAL_ONCE_CHECK); + + /** Queue of collected referents, drained on each call to evict stale {@link IdRef}s from {@link #SEEN}. */ + private static final ReferenceQueue<Message> Q = new ReferenceQueue<>(); + + /** Finish-unmarshalled instances, held weakly and keyed by identity so they vanish with the message. */ + private static final Set<IdRef> SEEN = ConcurrentHashMap.newKeySet(); + + /** */ + private MessageUnmarshalDedup() { + // No-op. + } + + /** + * @param msg Message about to be finish-unmarshalled. + * @param cacheMode {@code true} for the cache-aware pass, {@code false} for the cache-free pass; the two passes + * over one message are legitimate and tracked separately, so only a repeat of the same pass is reported. + * @return {@code true} if {@code msg} is not a {@link MarshallableMessage} or is finish-unmarshalled the first + * time in this pass. + */ + public static boolean firstUnmarshal(Message msg, boolean cacheMode) { + if (!(msg instanceof MarshallableMessage)) + return true; + + for (Reference<? extends Message> r; (r = Q.poll()) != null; ) + SEEN.remove(r); + + return SEEN.add(new IdRef(msg, cacheMode)); + } + + /** Weak reference to a message keyed by (identity, pass), so distinct messages and the two passes stay distinct. */ + private static final class IdRef extends WeakReference<Message> { + /** Referent identity hash folded with the pass, captured up front since the referent may be cleared later. */ + private final int hash; + + /** Unmarshal pass: cache-aware vs cache-free. Keeps the two legitimate passes over one message distinct. */ + private final boolean cacheMode; + + /** + * @param msg Tracked message. + * @param cacheMode Unmarshal pass. + */ + IdRef(Message msg, boolean cacheMode) { + super(msg, Q); + + this.cacheMode = cacheMode; + hash = 31 * System.identityHashCode(msg) + (cacheMode ? 1 : 0); + } + + /** {@inheritDoc} */ + @Override public int hashCode() { + return hash; + } + + /** {@inheritDoc} */ + @Override public boolean equals(Object o) { + if (this == o) + return true; + + if (!(o instanceof IdRef)) + return false; + + IdRef ref = (IdRef)o; + + Message m = get(); + + return m != null && m == ref.get() && cacheMode == ref.cacheMode; Review Comment: The `m != null` guard short-circuits before we ever compare two nulls: if this ref's referent is already cleared we return `false` without touching `ref.get()`. So two cleared refs never compare equal. ########## modules/core/src/main/java/org/apache/ignite/internal/managers/communication/MessageUnmarshalDedup.java: ########## @@ -0,0 +1,112 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.apache.ignite.internal.managers.communication; + +import java.lang.ref.Reference; +import java.lang.ref.ReferenceQueue; +import java.lang.ref.WeakReference; +import java.util.Set; +import java.util.concurrent.ConcurrentHashMap; +import org.apache.ignite.IgniteSystemProperties; +import org.apache.ignite.internal.MarshallableMessage; +import org.apache.ignite.plugin.extensions.communication.Message; +import org.apache.ignite.plugin.extensions.communication.MessageMarshaller; + +/** + * Detects a {@link MarshallableMessage} instance being finish-unmarshalled more than once within the same pass + * (cache-aware or cache-free) — a class-loader or receive-path bug. The two passes over one message (e.g. the + * generic {@code GridIoManager} pass plus a subsystem's cache-aware pass) are legitimate and tracked separately. + * Gated by {@link #ENABLED}, so it runs only under tests and is folded away in production. + * + * @see MessageMarshaller + */ +public class MessageUnmarshalDedup { + /** + * When {@code true}, the no-double-unmarshal check runs. {@code static final} so the JIT folds the guard away + * in production (even with assertions on); enabled only by tests via {@code IGNITE_MESSAGE_UNMARSHAL_ONCE_CHECK}. + */ + public static final boolean ENABLED = IgniteSystemProperties.getBoolean(IgniteSystemProperties.IGNITE_MESSAGE_UNMARSHAL_ONCE_CHECK); + + /** Queue of collected referents, drained on each call to evict stale {@link IdRef}s from {@link #SEEN}. */ + private static final ReferenceQueue<Message> Q = new ReferenceQueue<>(); + + /** Finish-unmarshalled instances, held weakly and keyed by identity so they vanish with the message. */ + private static final Set<IdRef> SEEN = ConcurrentHashMap.newKeySet(); + + /** */ + private MessageUnmarshalDedup() { + // No-op. + } + + /** + * @param msg Message about to be finish-unmarshalled. + * @param cacheMode {@code true} for the cache-aware pass, {@code false} for the cache-free pass; the two passes + * over one message are legitimate and tracked separately, so only a repeat of the same pass is reported. + * @return {@code true} if {@code msg} is not a {@link MarshallableMessage} or is finish-unmarshalled the first + * time in this pass. + */ + public static boolean firstUnmarshal(Message msg, boolean cacheMode) { + if (!(msg instanceof MarshallableMessage)) + return true; + + for (Reference<? extends Message> r; (r = Q.poll()) != null; ) + SEEN.remove(r); + + return SEEN.add(new IdRef(msg, cacheMode)); + } + + /** Weak reference to a message keyed by (identity, pass), so distinct messages and the two passes stay distinct. */ + private static final class IdRef extends WeakReference<Message> { + /** Referent identity hash folded with the pass, captured up front since the referent may be cleared later. */ + private final int hash; + + /** Unmarshal pass: cache-aware vs cache-free. Keeps the two legitimate passes over one message distinct. */ + private final boolean cacheMode; + + /** + * @param msg Tracked message. + * @param cacheMode Unmarshal pass. + */ + IdRef(Message msg, boolean cacheMode) { + super(msg, Q); Review Comment: The queue is needed — the flag is on for the *entire* suite (`GridAbstractTest` sets `IGNITE_MESSAGE_UNMARSHAL_ONCE_CHECK=true`), and `seen` is a static set, so without draining the collected weak-refs it grows unbounded across a test class. `MessageUnmarshalOnceTest` unmarshals only a couple of instances, so removing the queue passes there but would leak under a real run. Added a comment on the drain loop. ########## modules/core/src/main/java/org/apache/ignite/internal/managers/communication/MessageUnmarshalDedup.java: ########## @@ -0,0 +1,112 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.apache.ignite.internal.managers.communication; + +import java.lang.ref.Reference; +import java.lang.ref.ReferenceQueue; +import java.lang.ref.WeakReference; +import java.util.Set; +import java.util.concurrent.ConcurrentHashMap; +import org.apache.ignite.IgniteSystemProperties; +import org.apache.ignite.internal.MarshallableMessage; +import org.apache.ignite.plugin.extensions.communication.Message; +import org.apache.ignite.plugin.extensions.communication.MessageMarshaller; + +/** + * Detects a {@link MarshallableMessage} instance being finish-unmarshalled more than once within the same pass + * (cache-aware or cache-free) — a class-loader or receive-path bug. The two passes over one message (e.g. the + * generic {@code GridIoManager} pass plus a subsystem's cache-aware pass) are legitimate and tracked separately. + * Gated by {@link #ENABLED}, so it runs only under tests and is folded away in production. + * + * @see MessageMarshaller + */ +public class MessageUnmarshalDedup { Review Comment: It already exists — `MessageUnmarshalOnceTest` calls `firstUnmarshal` directly: the check is enabled suite-wide, a second unmarshal of the same instance in one pass is detected, and both legitimate passes (cache-free + cache-aware) are allowed. ########## modules/core/src/main/java/org/apache/ignite/internal/managers/communication/MessageUnmarshalDedup.java: ########## @@ -0,0 +1,112 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.apache.ignite.internal.managers.communication; + +import java.lang.ref.Reference; +import java.lang.ref.ReferenceQueue; +import java.lang.ref.WeakReference; +import java.util.Set; +import java.util.concurrent.ConcurrentHashMap; +import org.apache.ignite.IgniteSystemProperties; +import org.apache.ignite.internal.MarshallableMessage; +import org.apache.ignite.plugin.extensions.communication.Message; +import org.apache.ignite.plugin.extensions.communication.MessageMarshaller; + +/** + * Detects a {@link MarshallableMessage} instance being finish-unmarshalled more than once within the same pass + * (cache-aware or cache-free) — a class-loader or receive-path bug. The two passes over one message (e.g. the + * generic {@code GridIoManager} pass plus a subsystem's cache-aware pass) are legitimate and tracked separately. + * Gated by {@link #ENABLED}, so it runs only under tests and is folded away in production. + * + * @see MessageMarshaller + */ +public class MessageUnmarshalDedup { Review Comment: No — marshal is a DAG: shared sub-messages are marshalled once behind the companion-bytes guards and a repeat is an idempotent no-op, so a double *marshal* is legitimate. Only the unmarshal side is a tree where a second finish-unmarshal of the same instance is a real bug — that's why the check is unmarshal-only. ########## modules/core/src/main/java/org/apache/ignite/internal/managers/communication/MessageUnmarshalDedup.java: ########## @@ -0,0 +1,112 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.apache.ignite.internal.managers.communication; + +import java.lang.ref.Reference; +import java.lang.ref.ReferenceQueue; +import java.lang.ref.WeakReference; +import java.util.Set; +import java.util.concurrent.ConcurrentHashMap; +import org.apache.ignite.IgniteSystemProperties; +import org.apache.ignite.internal.MarshallableMessage; +import org.apache.ignite.plugin.extensions.communication.Message; +import org.apache.ignite.plugin.extensions.communication.MessageMarshaller; + +/** + * Detects a {@link MarshallableMessage} instance being finish-unmarshalled more than once within the same pass + * (cache-aware or cache-free) — a class-loader or receive-path bug. The two passes over one message (e.g. the + * generic {@code GridIoManager} pass plus a subsystem's cache-aware pass) are legitimate and tracked separately. + * Gated by {@link #ENABLED}, so it runs only under tests and is folded away in production. + * + * @see MessageMarshaller + */ +public class MessageUnmarshalDedup { Review Comment: Because a double *marshal* isn't a bug: marshalling is a DAG — shared sub-messages are marshalled once behind the companion-bytes guards, and re-marshalling is an idempotent no-op. Unmarshalling is a tree (deserialization yields fresh instances), so a second finish-unmarshal of the same instance in one pass is a genuine class-loader/receive-path bug — hence the check guards that direction only. -- 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]
