Copilot commented on code in PR #734:
URL:
https://github.com/apache/hugegraph-toolchain/pull/734#discussion_r3185889752
##########
hugegraph-loader/src/test/java/org/apache/hugegraph/loader/test/unit/DateUtilTest.java:
##########
@@ -70,4 +85,38 @@ public void testCheckTimeZone() {
// minutes 00-59 only
Assert.assertFalse(DateUtil.checkTimeZone("GMT+13:60"));
}
+
+ @Test
+ public void testConcurrentParseDateWithDifferentTimeZones() throws
InterruptedException {
+ int threads = 10;
+ int iterations = 100;
+ ExecutorService executor = Executors.newFixedThreadPool(threads);
+ CountDownLatch latch = new CountDownLatch(threads);
+ AtomicInteger errors = new AtomicInteger(0);
+
+ String dateStr = "2024-01-15 12:00:00";
+ String pattern = "yyyy-MM-dd HH:mm:ss";
+
+ for (int i = 0; i < threads; i++) {
+ final int threadId = i;
+ executor.submit(() -> {
+ try {
+ String timeZone = threadId % 2 == 0 ? "GMT+8" : "GMT+0";
+ for (int j = 0; j < iterations; j++) {
+ Date result = DateUtil.parse(dateStr, pattern,
timeZone);
+ if (result == null) {
+ errors.incrementAndGet();
+ }
+ }
Review Comment:
`testConcurrentParseDateWithDifferentTimeZones()` only asserts that
`DateUtil.parse()` returns non-null. The reported bug is incorrect parsing due
to timezone cross-talk, which would still return a non-null `Date`. To make
this test meaningful, assert that each thread always parses to the expected
epoch millis for its chosen timezone (e.g., results differ by 8 hours between
`GMT+8` and `GMT+0`), and count/propagate exceptions thrown inside tasks
(currently exceptions are swallowed by the executor and won’t fail the test).
##########
hugegraph-loader/src/main/java/org/apache/hugegraph/loader/util/DateUtil.java:
##########
@@ -18,16 +18,15 @@
package org.apache.hugegraph.loader.util;
import java.util.Date;
-import java.util.Map;
import java.util.TimeZone;
-import java.util.concurrent.ConcurrentHashMap;
import org.apache.hugegraph.date.SafeDateFormat;
import org.apache.hugegraph.loader.constant.Constants;
public final class DateUtil {
- private static final Map<String, SafeDateFormat> DATE_FORMATS = new
ConcurrentHashMap<>();
+ private static final ThreadLocal<java.util.HashMap<String,
SafeDateFormat>> DATE_FORMATS =
+ ThreadLocal.withInitial(java.util.HashMap::new);
Review Comment:
Using a static `ThreadLocal<HashMap<...>>` will retain one formatter map per
thread for the lifetime of the thread. In environments using long-lived thread
pools, this can increase memory usage proportional to (threads × distinct
patterns) and makes cached formatters effectively non-reclaimable until threads
exit. Consider an alternative cache structure (e.g., `ConcurrentHashMap<String,
ThreadLocal<SafeDateFormat>>` so the number of `ThreadLocal` instances is
bounded by patterns, or providing a way to clear the thread-local cache when
appropriate).
##########
hugegraph-loader/src/test/java/org/apache/hugegraph/loader/test/unit/DateUtilTest.java:
##########
@@ -17,13 +17,28 @@
package org.apache.hugegraph.loader.test.unit;
+import java.util.Date;
+import java.util.concurrent.CountDownLatch;
+import java.util.concurrent.ExecutorService;
+import java.util.concurrent.Executors;
+import java.util.concurrent.atomic.AtomicInteger;
+
import org.apache.hugegraph.loader.util.DateUtil;
import org.junit.Test;
import org.apache.hugegraph.testutil.Assert;
public class DateUtilTest {
+ @Test
+ public void testNowUsesDefaultTimeZone() {
+ String pattern = "Z";
+
+ DateUtil.parse("1970-01-01 +0000", "yyyy-MM-dd Z", "GMT");
Review Comment:
`testNowUsesDefaultTimeZone()` currently calls `DateUtil.parse()` with a
different date format (`yyyy-MM-dd Z`) than the one used by `DateUtil.now()`
(`Z`). This means the parse call doesn’t exercise the intended “timezone
pollution” scenario (formatter instance isn’t shared), so the test may pass
even if the regression returns. Consider either removing the parse call (and
renaming the test to reflect what it actually asserts), or using the same `df`
value for both calls so the test validates that a prior `parse()` timezone
change can’t affect `now()` for the same formatter.
--
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]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
