Aias00 commented on code in PR #7145:
URL: https://github.com/apache/shenyu/pull/7145#discussion_r4060322693
##########
shenyu-sync-data-center/shenyu-sync-data-consul/src/main/java/org/apache/shenyu/sync/data/consul/ConsulSyncDataService.java:
##########
@@ -60,9 +60,9 @@ public class ConsulSyncDataService extends
AbstractPathDataSyncService {
*/
private static final Logger LOG =
LoggerFactory.getLogger(ConsulSyncDataService.class);
- private final Map<String, Long> consulIndexes = new HashMap<>();
+ private final Map<String, Long> consulIndexes = new ConcurrentHashMap<>();
Review Comment:
Swapping `HashMap` for `ConcurrentHashMap` is the right call here:
`watcherData0(...)` schedules one self-rescheduling watcher per path on a
`ScheduledThreadPoolExecutor(7)`, so up to seven threads mutate these two maps
concurrently.
One thing this does *not* fix, worth tracking as a follow-up rather than
blocking: `watchConfigKeyValues` still does a check-then-act across the whole
map -
```java
if (!this.consulIndexes.containsValue(newIndex) &&
!currentIndex.equals(ConsulConstants.INIT_CONFIG_VERSION_INDEX)) { ... }
this.consulIndexes.put(watchPathRoot, newIndex);
```
`containsValue` is weakly consistent and the read-modify-write is not
atomic, so two watchers that observe the same `newIndex` can both pass the
guard and publish the same event twice. `ConcurrentHashMap` removes the
corruption risk (which is the real bug), but not the duplicate-publish race. If
you want to close that too, `compute`/`merge` on a per-path entry would be the
natural shape.
##########
shenyu-sync-data-center/shenyu-sync-data-consul/src/test/java/org/apache/shenyu/sync/data/consul/ConsulSyncDataServiceTest.java:
##########
@@ -131,8 +132,19 @@ public void testWatchConfigKeyValues() throws
NoSuchMethodException, IllegalAcce
final Field consulIndexes =
ConsulSyncDataService.class.getDeclaredField("consulIndexes");
consulIndexes.setAccessible(true);
final Map<String, Long> consulIndexesSource = (Map<String, Long>)
consulIndexes.get(consulSyncDataService);
- consulIndexesSource.put("/null", null);
+ consulIndexesSource.remove(watchPathRoot);
when(response.getConsulIndex()).thenReturn(2L);
Assertions.assertDoesNotThrow(() ->
watchConfigKeyValues.invoke(consulSyncDataService, watchPathRoot,
updateHandler, deleteHandler));
}
+
+ @Test
+ public void testWatcherStateUsesConcurrentMaps() throws
NoSuchFieldException, IllegalAccessException {
+ Field consulIndexesField =
ConsulSyncDataService.class.getDeclaredField("consulIndexes");
+ consulIndexesField.setAccessible(true);
+ Field cacheDataField =
ConsulSyncDataService.class.getDeclaredField("cacheConsulDataKeyMap");
+ cacheDataField.setAccessible(true);
+
+ Assertions.assertTrue(consulIndexesField.get(consulSyncDataService)
instanceof ConcurrentHashMap);
Review Comment:
This assertion locks the test to a concrete implementation type rather than
to behaviour. It will fail if someone later changes the field to
`ConcurrentMap<String, Long>`, wraps it with `Collections.synchronizedMap`, or
switches to a different concurrent map - even though the production behaviour
would still be correct. It also proves nothing about concurrency: a map can be
a `ConcurrentHashMap` and the surrounding code can still be unsafe.
`remove(watchPathRoot)` above is the valuable part of this test - it
actually drives the `Objects.isNull(currentIndex) -> INIT_CONFIG_VERSION_INDEX`
branch, whereas the old `put("/null", null)` wrote a key the code never read. I
would keep that and either drop this test or make it behavioural, e.g. mutate
the two maps from N threads and assert no lost updates / no exception. If you
keep a structural check, at least assert assignability to `ConcurrentMap`
instead of `instanceof ConcurrentHashMap`.
--
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]
