From b2aace3de9f547b08adc198ee13843b48c26b66d Mon Sep 17 00:00:00 2001 From: Rui Gu Date: Tue, 3 Oct 2017 17:24:56 +0100 Subject: [PATCH 1/2] Better collection handling for RedissonReference #1084 --- .../redisson/command/CommandAsyncService.java | 218 +++++++++++------- 1 file changed, 129 insertions(+), 89 deletions(-) diff --git a/redisson/src/main/java/org/redisson/command/CommandAsyncService.java b/redisson/src/main/java/org/redisson/command/CommandAsyncService.java index cadcecefd..a476efeed 100644 --- a/redisson/src/main/java/org/redisson/command/CommandAsyncService.java +++ b/redisson/src/main/java/org/redisson/command/CommandAsyncService.java @@ -864,109 +864,149 @@ public class CommandAsyncService implements CommandAsyncExecutor { } private void handleReference(RPromise mainPromise, R res) { - if (res instanceof List) { - List r = (List) res; + try { + mainPromise.trySuccess(tryHandleReference(res)); + } catch (Exception e) { + //fall back and let other part of the code handle the type conversion. + mainPromise.trySuccess(res); + } + } + + private T tryHandleReference(T o) { + boolean hasConversion = false; + if (o instanceof List) { + List r = (List) o; for (int i = 0; i < r.size(); i++) { - if (r.get(i) instanceof RedissonReference) { - r.set(i, fromReference(r.get(i))); - } else if (r.get(i) instanceof ScoredEntry && ((ScoredEntry) r.get(i)).getValue() instanceof RedissonReference) { - ScoredEntry se = ((ScoredEntry) r.get(i)); - se = new ScoredEntry(se.getScore(), fromReference(se.getValue())); - r.set(i, se); + Object ref = tryHandleReference0(r.get(i)); + if (ref != r.get(i)) { + r.set(i, ref); } } - mainPromise.trySuccess(res); - } else if (res instanceof Set) { - Set r = (Set) res; - LinkedHashSet converted = new LinkedHashSet(); - for (Object o : r) { - if (o instanceof RedissonReference) { - converted.add(fromReference(o)); - } else if (o instanceof ScoredEntry && ((ScoredEntry) o).getValue() instanceof RedissonReference) { - ScoredEntry se = ((ScoredEntry) o); - se = new ScoredEntry(se.getScore(), fromReference(se.getValue())); - converted.add(se); - } else if (o instanceof Map.Entry) { - Map.Entry old = (Map.Entry) o; - Object key = old.getKey(); - if (key instanceof RedissonReference) { - key = fromReference(key); - } - Object value = old.getValue(); - if (value instanceof RedissonReference) { - value = fromReference(value); - } - converted.add(new AbstractMap.SimpleEntry(key, value)); + return o; + } else if (o instanceof Set) { + Set set, r = (Set) o; + boolean isLinkedSet = o instanceof LinkedHashSet; + try { + set = (Set) o.getClass().getConstructor().newInstance(); + } catch (Exception exception) { + set = new LinkedHashSet(); + } + for (Object i : r) { + Object ref = tryHandleReference0(i); + //Not testing for ref changes because r.add(ref) below needs to + //fail on the first iteration to be able to perform fall back + //if failure happens. + // + //Assuming the failure reason is systematic such as put method + //is not supported or implemented, and not an occasional issue + //like only one element fails. + if (isLinkedSet) { + set.add(ref); } else { - converted.add(o); + try { + r.add(ref); + set.add(i); + } catch (Exception e) { + //r is not supporting add operation, like + //LinkedHashMap$LinkedEntrySet and others. + //fall back to use a new set. + isLinkedSet = true; + set.add(ref); + } } + hasConversion |= ref != i; } - mainPromise.trySuccess((R) converted); - } else if (res instanceof Map) { - Map map = (Map) res; - LinkedHashMap converted = new LinkedHashMap(); - for (Map.Entry e : map.entrySet()) { - Object value = e.getValue(); - if (e.getValue() instanceof RedissonReference) { - value = fromReference(e.getValue()); - } - Object key = e.getKey(); - if (e.getKey() instanceof RedissonReference) { - key = fromReference(e.getKey()); - } - converted.put(key, value); + + if (!hasConversion) { + return o; + } else if (isLinkedSet) { + return (T) set; + } else if (!set.isEmpty()) { + r.removeAll(set); } - mainPromise.trySuccess((R) converted); - } else if (res instanceof ListScanResult) { - List r = ((ListScanResult) res).getValues(); - for (int i = 0; i < r.size(); i++) { - Object obj = r.get(i); - if (!(obj instanceof ScanObjectEntry)) { - break; - } - ScanObjectEntry e = r.get(i); - if (e.getObj() instanceof RedissonReference) { - r.set(i, new ScanObjectEntry(e.getBuf(), fromReference(e.getObj()))); - } else if (e.getObj() instanceof ScoredEntry && ((ScoredEntry) e.getObj()).getValue() instanceof RedissonReference) { - ScoredEntry se = ((ScoredEntry) e.getObj()); - se = new ScoredEntry(se.getScore(), fromReference(se.getValue())); - r.set(i, new ScanObjectEntry(e.getBuf(), se)); + return o; + } else if (o instanceof Map) { + Map map, r = (Map) o; + boolean isLinkedMap = o instanceof LinkedHashMap; + try { + map = (Map) o.getClass().getConstructor().newInstance(); + } catch (Exception e) { + map = new LinkedHashMap(); + } + for (Map.Entry e : r.entrySet()) { + Map.Entry ref = tryHandleReference0(e); + //Not testing for ref changes because r.put(ref.getKey(), ref.getValue()) + //below needs to fail on the first iteration to be able to + //perform fall back if failure happens. + // + //Assuming the failure reason is systematic such as put method + //is not supported or implemented, and not an occasional issue + //like only one element fails. + if (isLinkedMap) { + map.put(ref.getKey(), ref.getValue()); + } else { + try { + r.put(ref.getKey(), ref.getValue()); + if (e.getKey() != ref.getKey()) { + map.put(e.getKey(), e.getValue()); + } + } catch (Exception ex) { + //r is not supporting put operation. fall back to use + //a new map. + isLinkedMap = true; + map.put(ref.getKey(), ref.getValue()); + } } + hasConversion |= ref != e; } - mainPromise.trySuccess(res); - } else if (res instanceof MapScanResult) { - MapScanResult scanResult = (MapScanResult) res; - Map map = ((MapScanResult) res).getMap(); - LinkedHashMap converted = new LinkedHashMap(); - boolean hasConversion = false; - for (Map.Entry e : map.entrySet()) { - ScanObjectEntry value = e.getValue(); - if (e.getValue().getObj() instanceof RedissonReference) { - value = new ScanObjectEntry(e.getValue().getBuf(), fromReference(e.getValue().getObj())); - hasConversion = true; - } - ScanObjectEntry key = e.getKey(); - if (e.getKey().getObj() instanceof RedissonReference) { - key = new ScanObjectEntry(e.getKey().getBuf(), fromReference(e.getKey().getObj())); - hasConversion = true; - } - converted.put(key, value); + + if (!hasConversion) { + return o; + } else if (isLinkedMap) { + return (T) map; + } else if (!map.isEmpty()) { + r.keySet().removeAll(map.keySet()); } - if (hasConversion) { - MapScanResult newScanResult = new MapScanResult(scanResult.getPos(), converted); + return o; + } else if (o instanceof ListScanResult) { + tryHandleReference(((ListScanResult) o).getValues()); + return o; + } else if (o instanceof MapScanResult) { + MapScanResult scanResult = (MapScanResult) o; + Map oldMap = ((MapScanResult) o).getMap(); + Map map = tryHandleReference(oldMap); + if (map != oldMap) { + MapScanResult newScanResult + = new MapScanResult(scanResult.getPos(), map); newScanResult.setRedisClient(scanResult.getRedisClient()); - mainPromise.trySuccess((R) newScanResult); + return (T) newScanResult; } else { - mainPromise.trySuccess((R) res); - } - } else if (res instanceof RedissonReference) { - try { - mainPromise.trySuccess(this.fromReference(res)); - } catch (Exception exception) { - mainPromise.trySuccess(res);//fallback + return o; } } else { - mainPromise.trySuccess(res); + return tryHandleReference0(o); + } + } + + private T tryHandleReference0(T o) { + if (o instanceof RedissonReference) { + return fromReference(o); + } else if (o instanceof ScoredEntry && ((ScoredEntry) o).getValue() instanceof RedissonReference) { + ScoredEntry se = ((ScoredEntry) o); + return (T) new ScoredEntry(se.getScore(), fromReference(se.getValue())); + } else if (o instanceof ScanObjectEntry) { + ScanObjectEntry keyScan = (ScanObjectEntry) o; + Object obj = tryHandleReference0(keyScan.getObj()); + return obj != keyScan.getObj() ? (T) new ScanObjectEntry(keyScan.getBuf(), obj) : o; + } else if (o instanceof Map.Entry) { + Map.Entry old = (Map.Entry) o; + Object key = tryHandleReference0(old.getKey()); + Object value = tryHandleReference0(old.getValue()); + return value != old.getValue() || key != old.getKey() + ? (T) new AbstractMap.SimpleEntry(key, value) + : o; + } else { + return o; } } From 9edbb5efde88520ba19748c152c03e9eb95b2d81 Mon Sep 17 00:00:00 2001 From: Rui Gu Date: Tue, 3 Oct 2017 17:28:47 +0100 Subject: [PATCH 2/2] parameter renamed to avoid confusion --- .../redisson/command/CommandAsyncService.java | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/redisson/src/main/java/org/redisson/command/CommandAsyncService.java b/redisson/src/main/java/org/redisson/command/CommandAsyncService.java index a476efeed..e83832b21 100644 --- a/redisson/src/main/java/org/redisson/command/CommandAsyncService.java +++ b/redisson/src/main/java/org/redisson/command/CommandAsyncService.java @@ -885,7 +885,7 @@ public class CommandAsyncService implements CommandAsyncExecutor { return o; } else if (o instanceof Set) { Set set, r = (Set) o; - boolean isLinkedSet = o instanceof LinkedHashSet; + boolean useNewSet = o instanceof LinkedHashSet; try { set = (Set) o.getClass().getConstructor().newInstance(); } catch (Exception exception) { @@ -900,7 +900,7 @@ public class CommandAsyncService implements CommandAsyncExecutor { //Assuming the failure reason is systematic such as put method //is not supported or implemented, and not an occasional issue //like only one element fails. - if (isLinkedSet) { + if (useNewSet) { set.add(ref); } else { try { @@ -910,7 +910,7 @@ public class CommandAsyncService implements CommandAsyncExecutor { //r is not supporting add operation, like //LinkedHashMap$LinkedEntrySet and others. //fall back to use a new set. - isLinkedSet = true; + useNewSet = true; set.add(ref); } } @@ -919,7 +919,7 @@ public class CommandAsyncService implements CommandAsyncExecutor { if (!hasConversion) { return o; - } else if (isLinkedSet) { + } else if (useNewSet) { return (T) set; } else if (!set.isEmpty()) { r.removeAll(set); @@ -927,7 +927,7 @@ public class CommandAsyncService implements CommandAsyncExecutor { return o; } else if (o instanceof Map) { Map map, r = (Map) o; - boolean isLinkedMap = o instanceof LinkedHashMap; + boolean useNewMap = o instanceof LinkedHashMap; try { map = (Map) o.getClass().getConstructor().newInstance(); } catch (Exception e) { @@ -942,7 +942,7 @@ public class CommandAsyncService implements CommandAsyncExecutor { //Assuming the failure reason is systematic such as put method //is not supported or implemented, and not an occasional issue //like only one element fails. - if (isLinkedMap) { + if (useNewMap) { map.put(ref.getKey(), ref.getValue()); } else { try { @@ -953,7 +953,7 @@ public class CommandAsyncService implements CommandAsyncExecutor { } catch (Exception ex) { //r is not supporting put operation. fall back to use //a new map. - isLinkedMap = true; + useNewMap = true; map.put(ref.getKey(), ref.getValue()); } } @@ -962,7 +962,7 @@ public class CommandAsyncService implements CommandAsyncExecutor { if (!hasConversion) { return o; - } else if (isLinkedMap) { + } else if (useNewMap) { return (T) map; } else if (!map.isEmpty()) { r.keySet().removeAll(map.keySet());