Copilot commented on code in PR #1872: URL: https://github.com/apache/fluss/pull/1872#discussion_r2642098239
########## fluss-server/src/test/java/org/apache/fluss/server/utils/timer/TimerTaskEntryTest.java: ########## @@ -0,0 +1,119 @@ +/* + * 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.fluss.server.utils.timer; + +import org.junit.jupiter.api.Test; + +import java.util.concurrent.CountDownLatch; +import java.util.concurrent.atomic.AtomicInteger; +import java.util.concurrent.atomic.AtomicReference; + +import static org.assertj.core.api.Assertions.assertThat; + +/** Tests for {@link org.apache.fluss.server.utils.timer.TimerTaskEntry}. */ +public class TimerTaskEntryTest { + + @Test + void testRemoveEnsuresCurrentListNullSafety() throws InterruptedException { + // Create two lists to reproduce the values that we are working + // with being added/removed. We will oscillate between adding + // and removing these elements until we encounter a NPE + AtomicInteger sharedTaskCounter = new AtomicInteger(0); + TimerTaskList primaryList = new TimerTaskList(sharedTaskCounter); + TimerTaskList secondaryList = new TimerTaskList(sharedTaskCounter); + + // Set up our initial task that will handle coordinating this + // reproduction behavior + TestTask task = new TestTask(0L); + TimerTaskEntry entry = new TimerTaskEntry(task, 10L); + primaryList.add(entry); + + // Container for any NullPointerException caught during remove() + AtomicReference<NullPointerException> thrownException = new AtomicReference<>(); + + // Latch to handle coordinating addition/removal threads + CountDownLatch latch = new CountDownLatch(1); + + // Create a thread responsible for continually removing entries, which + // will be responsible for triggering the exception + Thread removalThread = + new Thread( + () -> { + try { + latch.await(); + // Continually remove elements from the task (forward-oscillation) + for (int i = 0; i < 10000; i++) { + entry.remove(); + } + } catch (NullPointerException e) { + thrownException.set(e); + } catch (InterruptedException e) { + Thread.currentThread().interrupt(); + } + }); + + // Create a separate thread for adding the entry while the removal thread is + // still executing which results in the expected null reference + Thread additionThread = + new Thread( + () -> { + try { + // Wait for the initial removal to complete + latch.await(); + // Add the entry from our separate list while the removal thread is + // still verifying the condition (resulting in our null list within + // the internal removal call, and our exception) + for (int i = 0; i < 10000; i++) { + // Determine which list to add to the task + // (backwards-oscillation) + if (entry.list == null) { + secondaryList.add(entry); + } else if (entry.list == secondaryList) { + primaryList.add(entry); + } + Thread.yield(); + } + } catch (InterruptedException e) { + Thread.currentThread().interrupt(); + } + }); + + // Start both threads + removalThread.start(); + additionThread.start(); + + // Release both threads to trigger our race condition + latch.countDown(); + + // Wait for threads to complete + removalThread.join(); + additionThread.join(); + + // Assert that no exception was originated + assertThat(thrownException.get()).isNull(); Review Comment: The test name suggests it ensures null safety, but the assertion on line 108 only verifies that no NullPointerException was thrown. The test doesn't verify that the entry was actually removed correctly or that the task counter has the expected value. Consider adding assertions to verify the final state of the entry (e.g., entry.list should be null) and the task counter to ensure the removal operations completed correctly. ########## fluss-server/src/test/java/org/apache/fluss/server/utils/timer/TimerTaskEntryTest.java: ########## @@ -0,0 +1,119 @@ +/* + * 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.fluss.server.utils.timer; + +import org.junit.jupiter.api.Test; + +import java.util.concurrent.CountDownLatch; +import java.util.concurrent.atomic.AtomicInteger; +import java.util.concurrent.atomic.AtomicReference; + +import static org.assertj.core.api.Assertions.assertThat; + +/** Tests for {@link org.apache.fluss.server.utils.timer.TimerTaskEntry}. */ +public class TimerTaskEntryTest { + + @Test + void testRemoveEnsuresCurrentListNullSafety() throws InterruptedException { + // Create two lists to reproduce the values that we are working + // with being added/removed. We will oscillate between adding + // and removing these elements until we encounter a NPE + AtomicInteger sharedTaskCounter = new AtomicInteger(0); + TimerTaskList primaryList = new TimerTaskList(sharedTaskCounter); + TimerTaskList secondaryList = new TimerTaskList(sharedTaskCounter); + + // Set up our initial task that will handle coordinating this + // reproduction behavior + TestTask task = new TestTask(0L); + TimerTaskEntry entry = new TimerTaskEntry(task, 10L); + primaryList.add(entry); + + // Container for any NullPointerException caught during remove() + AtomicReference<NullPointerException> thrownException = new AtomicReference<>(); + + // Latch to handle coordinating addition/removal threads + CountDownLatch latch = new CountDownLatch(1); + + // Create a thread responsible for continually removing entries, which + // will be responsible for triggering the exception + Thread removalThread = + new Thread( + () -> { + try { + latch.await(); + // Continually remove elements from the task (forward-oscillation) + for (int i = 0; i < 10000; i++) { + entry.remove(); + } + } catch (NullPointerException e) { + thrownException.set(e); + } catch (InterruptedException e) { + Thread.currentThread().interrupt(); + } + }); + + // Create a separate thread for adding the entry while the removal thread is + // still executing which results in the expected null reference + Thread additionThread = + new Thread( + () -> { + try { + // Wait for the initial removal to complete + latch.await(); + // Add the entry from our separate list while the removal thread is + // still verifying the condition (resulting in our null list within + // the internal removal call, and our exception) + for (int i = 0; i < 10000; i++) { + // Determine which list to add to the task + // (backwards-oscillation) + if (entry.list == null) { + secondaryList.add(entry); + } else if (entry.list == secondaryList) { + primaryList.add(entry); + } Review Comment: The test directly accesses the package-private field 'entry.list' to determine which list to add the entry to. This creates a tight coupling between the test and the internal implementation details. Consider using public methods or adding a package-private method to check the list state if necessary, rather than directly accessing the field. This would make the test more maintainable if the internal structure changes. ########## fluss-server/src/test/java/org/apache/fluss/server/utils/timer/TimerTaskEntryTest.java: ########## @@ -0,0 +1,119 @@ +/* + * 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.fluss.server.utils.timer; + +import org.junit.jupiter.api.Test; + +import java.util.concurrent.CountDownLatch; +import java.util.concurrent.atomic.AtomicInteger; +import java.util.concurrent.atomic.AtomicReference; + +import static org.assertj.core.api.Assertions.assertThat; + +/** Tests for {@link org.apache.fluss.server.utils.timer.TimerTaskEntry}. */ +public class TimerTaskEntryTest { + + @Test + void testRemoveEnsuresCurrentListNullSafety() throws InterruptedException { + // Create two lists to reproduce the values that we are working + // with being added/removed. We will oscillate between adding + // and removing these elements until we encounter a NPE + AtomicInteger sharedTaskCounter = new AtomicInteger(0); + TimerTaskList primaryList = new TimerTaskList(sharedTaskCounter); + TimerTaskList secondaryList = new TimerTaskList(sharedTaskCounter); + + // Set up our initial task that will handle coordinating this + // reproduction behavior + TestTask task = new TestTask(0L); + TimerTaskEntry entry = new TimerTaskEntry(task, 10L); + primaryList.add(entry); + + // Container for any NullPointerException caught during remove() + AtomicReference<NullPointerException> thrownException = new AtomicReference<>(); + + // Latch to handle coordinating addition/removal threads + CountDownLatch latch = new CountDownLatch(1); + + // Create a thread responsible for continually removing entries, which + // will be responsible for triggering the exception + Thread removalThread = + new Thread( + () -> { + try { + latch.await(); + // Continually remove elements from the task (forward-oscillation) + for (int i = 0; i < 10000; i++) { + entry.remove(); + } + } catch (NullPointerException e) { + thrownException.set(e); + } catch (InterruptedException e) { + Thread.currentThread().interrupt(); + } + }); + + // Create a separate thread for adding the entry while the removal thread is + // still executing which results in the expected null reference + Thread additionThread = + new Thread( + () -> { + try { + // Wait for the initial removal to complete + latch.await(); + // Add the entry from our separate list while the removal thread is + // still verifying the condition (resulting in our null list within + // the internal removal call, and our exception) + for (int i = 0; i < 10000; i++) { + // Determine which list to add to the task + // (backwards-oscillation) + if (entry.list == null) { + secondaryList.add(entry); + } else if (entry.list == secondaryList) { Review Comment: The conditional logic for adding entries is incomplete. If entry.list equals primaryList, neither condition (entry.list == null or entry.list == secondaryList) will be true, and no add operation occurs for that iteration. This means the test may not consistently reproduce the race condition it's designed to test. Consider handling all possible states: when list is null, when it's primaryList, and when it's secondaryList. ```suggestion TimerTaskList currentList = entry.list; if (currentList == null || currentList == primaryList) { // If the entry is not in any list or is in the primary list, // move it to the secondary list. secondaryList.add(entry); } else if (currentList == secondaryList) { // If the entry is in the secondary list, move it to the primary list. ``` ########## fluss-server/src/test/java/org/apache/fluss/server/utils/timer/TimerTaskEntryTest.java: ########## @@ -0,0 +1,119 @@ +/* + * 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.fluss.server.utils.timer; + +import org.junit.jupiter.api.Test; + +import java.util.concurrent.CountDownLatch; +import java.util.concurrent.atomic.AtomicInteger; +import java.util.concurrent.atomic.AtomicReference; + +import static org.assertj.core.api.Assertions.assertThat; + +/** Tests for {@link org.apache.fluss.server.utils.timer.TimerTaskEntry}. */ +public class TimerTaskEntryTest { + + @Test + void testRemoveEnsuresCurrentListNullSafety() throws InterruptedException { + // Create two lists to reproduce the values that we are working + // with being added/removed. We will oscillate between adding + // and removing these elements until we encounter a NPE + AtomicInteger sharedTaskCounter = new AtomicInteger(0); + TimerTaskList primaryList = new TimerTaskList(sharedTaskCounter); + TimerTaskList secondaryList = new TimerTaskList(sharedTaskCounter); + + // Set up our initial task that will handle coordinating this + // reproduction behavior + TestTask task = new TestTask(0L); + TimerTaskEntry entry = new TimerTaskEntry(task, 10L); + primaryList.add(entry); + + // Container for any NullPointerException caught during remove() + AtomicReference<NullPointerException> thrownException = new AtomicReference<>(); + + // Latch to handle coordinating addition/removal threads + CountDownLatch latch = new CountDownLatch(1); + + // Create a thread responsible for continually removing entries, which + // will be responsible for triggering the exception + Thread removalThread = + new Thread( + () -> { + try { + latch.await(); + // Continually remove elements from the task (forward-oscillation) + for (int i = 0; i < 10000; i++) { + entry.remove(); + } + } catch (NullPointerException e) { + thrownException.set(e); + } catch (InterruptedException e) { + Thread.currentThread().interrupt(); + } + }); + + // Create a separate thread for adding the entry while the removal thread is + // still executing which results in the expected null reference + Thread additionThread = + new Thread( + () -> { + try { + // Wait for the initial removal to complete + latch.await(); + // Add the entry from our separate list while the removal thread is Review Comment: The comment has a grammatical error. It should read "Add the entry to our separate list" instead of "Add the entry from our separate list" since the operation is adding to a list, not adding from a list. ```suggestion // Add the entry to our separate list while the removal thread is ``` -- 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]
