anton-vinogradov commented on code in PR #13095: URL: https://github.com/apache/ignite/pull/13095#discussion_r3588944568
########## 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: Renamed — to `MessageUnmarshalOnceCheck` rather than `Deduplicator`: it detects/asserts a double unmarshal, it doesn't remove duplicates, and the new name lines up with the `IGNITE_MESSAGE_UNMARSHAL_ONCE_CHECK` property and the `MessageUnmarshalOnceTest`. ########## 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<>(); Review Comment: Renamed `Q` → `refQueue`. ########## 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) { Review Comment: Done — the `IdRef` constructor is `private` now. ########## 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)) Review Comment: Done — `o instanceof IdRef ref` pattern. ########## 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 Review Comment: Shortened the class javadoc. ########## 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. */ Review Comment: Rephrased — "Unmarshal pass: `true` = cache-aware, `false` = cache-free." -- 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]
