Skip to content

Commit 794ee77

Browse files
katysaintinkatysaintin
committed
Fix bug when Factory generate a core-name different than pvname
1 parent b6248e4 commit 794ee77

File tree

7 files changed

+263
-20
lines changed

7 files changed

+263
-20
lines changed

‎core/pv/src/main/java/org/phoebus/pv/PVPool.java‎

Lines changed: 45 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -178,6 +178,11 @@ public static Set<String> getNameVariants(final String name, final String [] equ
178178
* Otherwise, two threads concurrently looking for a new PV would both add it.
179179
*/
180180
final private static RefCountMap<String, PV> pool = new RefCountMap<>();
181+
/**
182+
* Manage Factory the provide PV named different of the core name
183+
* Map<PVName,coreName>
184+
*/
185+
final private static Map<String, String> coreNameMap = new HashMap<>();
181186

182187
/** Singleton */
183188
private PVPool()
@@ -210,8 +215,25 @@ public static PV getPV(final String name) throws Exception
210215
if (factory == null)
211216
throw new Exception(_name + " has unknown PV type '" + type_name.type + "'");
212217

213-
final String core_name = factory.getCoreName(_name);
214-
final ReferencedEntry<PV> ref = pool.createOrGet(core_name, () -> createPV(factory, _name, type_name.name));
218+
String core_name = factory.getCoreName(_name);
219+
220+
//Check if there is an existing PV which named different from core name
221+
final String uniqueKey = !_name.startsWith(type_name.type + "://") ? type_name.type + "://" + _name : _name;
222+
//Some factory not manage a unique prefix such as ca and pva and create twice PV for ca://pv_name and pv_name
223+
if(!core_name.startsWith(type_name.type + "://")){
224+
core_name = factory.getCoreName(uniqueKey);
225+
}
226+
227+
if(coreNameMap.containsKey(uniqueKey)) {
228+
core_name = coreNameMap.get(uniqueKey);
229+
}
230+
231+
final ReferencedEntry<PV> ref = pool.createOrGet(core_name, () -> createPV(factory, _name , type_name.name));
232+
String pvName = ref.getEntry().getName();
233+
//Add the corresponding core_name for a given pvName
234+
if(!coreNameMap.containsKey(uniqueKey) && !core_name.equals(uniqueKey) && !core_name.equals(pvName)) {
235+
coreNameMap.put(uniqueKey,core_name);
236+
}
215237
logger.log(Level.CONFIG, () -> "PV '" + ref.getEntry().getName() + "' references: " + ref.getReferences());
216238
return ref.getEntry();
217239
}
@@ -232,14 +254,28 @@ private static PV createPV(PVFactory factory, final String name, final String ba
232254
/** @param pv PV to be released */
233255
public static void releasePV(final PV pv)
234256
{
235-
final int references = pool.release(pv.getName());
236-
if (references <= 0)
237-
{
238-
pv.close();
239-
logger.log(Level.CONFIG, () -> "PV '" + pv.getName() + "' closed");
257+
//First find the pv
258+
String entryKey = pool.getReferencedEntryKey(pv);
259+
if(entryKey != null) {
260+
final int references = pool.release(entryKey);
261+
if (references <= 0)
262+
{
263+
pv.close();
264+
if(coreNameMap.containsKey(entryKey)) {
265+
coreNameMap.remove(entryKey);
266+
}
267+
logger.log(Level.CONFIG, () -> "PV '" + pv.getName() + "' closed");
268+
}
269+
else
270+
logger.log(Level.CONFIG, () -> "PV '" + pv.getName() + "' remaining references: " + references);
240271
}
241-
else
242-
logger.log(Level.CONFIG, () -> "PV '" + pv.getName() + "' remaining references: " + references);
272+
else {
273+
logger.log(Level.WARNING, "No reference found for " + pv.getName());
274+
}
275+
}
276+
277+
public static ReferencedEntry<PV> getReferenceEntry(final PV pv) {
278+
return pool.getReferencedEntry(pv);
243279
}
244280

245281
/** @return PVs currently in the pool with reference count information */

‎core/pv/src/main/java/org/phoebus/pv/RefCountMap.java‎

Lines changed: 35 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -13,7 +13,9 @@
1313
import java.util.Collections;
1414
import java.util.HashMap;
1515
import java.util.Map;
16+
import java.util.Map.Entry;
1617
import java.util.Objects;
18+
import java.util.Set;
1719
import java.util.concurrent.atomic.AtomicInteger;
1820
import java.util.function.Supplier;
1921
import java.util.logging.Level;
@@ -137,7 +139,7 @@ public int release(final K key)
137139
return refs;
138140
}
139141
}
140-
142+
141143
/** @return Entries in map */
142144
public Collection<ReferencedEntry<E>> getEntries()
143145
{
@@ -146,4 +148,36 @@ public Collection<ReferencedEntry<E>> getEntries()
146148
return Collections.unmodifiableCollection(map.values());
147149
}
148150
}
151+
152+
/**
153+
* @param entry
154+
* @return the Key for a given entry based on hashcode
155+
*/
156+
public K getReferencedEntryKey(E entry) {
157+
K entryKey = null;
158+
Set<Entry<K, ReferencedEntry<E>>> entrySet = map.entrySet();
159+
for(Entry<K, ReferencedEntry<E>> ref : entrySet) {
160+
if(ref.getValue().getEntry().hashCode() == entry.hashCode()) {
161+
entryKey = ref.getKey();
162+
break;
163+
}
164+
}
165+
return entryKey;
166+
}
167+
168+
/**
169+
* @param entry
170+
* @return the Key for a given entry based on hashcode
171+
*/
172+
public ReferencedEntry<E> getReferencedEntry(E entry) {
173+
ReferencedEntry<E> refEntry = null;
174+
Collection<ReferencedEntry<E>> values = map.values();
175+
for(ReferencedEntry<E> ref : values) {
176+
if(ref.getEntry().hashCode() == entry.hashCode()) {
177+
refEntry = ref;
178+
break;
179+
}
180+
}
181+
return refEntry;
182+
}
149183
}

‎core/pv/src/test/java/org/phoebus/pv/LocalPVTest.java‎

Lines changed: 43 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -7,16 +7,6 @@
77
******************************************************************************/
88
package org.phoebus.pv;
99

10-
import io.reactivex.rxjava3.disposables.Disposable;
11-
import org.epics.vtype.VDouble;
12-
import org.epics.vtype.VType;
13-
import org.junit.jupiter.api.Test;
14-
import org.phoebus.pv.loc.LocalPVFactory;
15-
import org.phoebus.pv.loc.ValueHelper;
16-
17-
import java.util.concurrent.atomic.AtomicInteger;
18-
import java.util.concurrent.atomic.AtomicReference;
19-
2010
import static org.hamcrest.CoreMatchers.containsString;
2111
import static org.hamcrest.CoreMatchers.equalTo;
2212
import static org.hamcrest.CoreMatchers.instanceOf;
@@ -25,6 +15,19 @@
2515
import static org.hamcrest.MatcherAssert.assertThat;
2616
import static org.junit.jupiter.api.Assertions.fail;
2717

18+
import java.util.Collection;
19+
import java.util.concurrent.atomic.AtomicInteger;
20+
import java.util.concurrent.atomic.AtomicReference;
21+
22+
import org.epics.vtype.VDouble;
23+
import org.epics.vtype.VType;
24+
import org.junit.jupiter.api.Test;
25+
import org.phoebus.pv.RefCountMap.ReferencedEntry;
26+
import org.phoebus.pv.loc.LocalPVFactory;
27+
import org.phoebus.pv.loc.ValueHelper;
28+
29+
import io.reactivex.rxjava3.disposables.Disposable;
30+
2831
/**
2932
* @author Kay Kasemir
3033
*/
@@ -149,5 +152,35 @@ public void testLocalPVNotifications() throws Exception {
149152
sub.dispose();
150153
PVPool.releasePV(pv);
151154
}
155+
156+
/**
157+
* Test LocalPV when setting local as default datasource
158+
*/
159+
@Test
160+
public void testRelasePV() throws Exception {
161+
162+
Collection<ReferencedEntry<PV>> pvReferences = PVPool.getPVReferences();
163+
164+
int initSize = pvReferences != null ? pvReferences.size() : 0;
165+
//Test for Write action on default pv different from ca see PR
166+
//https://github.com/ControlSystemStudio/phoebus/pull/3412
167+
//Force default data source to loc://
168+
PVPool.default_type = LocalPVFactory.TYPE;
169+
String pv_name = "my_pv";
170+
171+
final PV pv = PVPool.getPV(pv_name);
172+
173+
pvReferences = PVPool.getPVReferences();
174+
175+
//Check that there is 1 reference
176+
assertThat(pvReferences.size() , equalTo(initSize + 1));
177+
178+
PVPool.releasePV(pv);
179+
180+
pvReferences = PVPool.getPVReferences();
181+
182+
//Check that there is no reference anymore
183+
assertThat(pvReferences.size() , equalTo(initSize));
184+
}
152185

153186
}

‎core/pv/src/test/java/org/phoebus/pv/PVPoolTest.java‎

Lines changed: 91 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -10,13 +10,15 @@
1010
import static org.hamcrest.CoreMatchers.equalTo;
1111
import static org.hamcrest.CoreMatchers.hasItem;
1212
import static org.hamcrest.MatcherAssert.assertThat;
13+
import static org.junit.jupiter.api.Assertions.assertNull;
1314

1415
import java.util.Collection;
1516
import java.util.Set;
1617
import java.util.prefs.Preferences;
1718

1819
import org.junit.jupiter.api.Test;
1920
import org.phoebus.pv.PVPool.TypedName;
21+
import org.phoebus.pv.RefCountMap.ReferencedEntry;
2022

2123
/** @author Kay Kasemir */
2224
@SuppressWarnings("nls")
@@ -100,6 +102,95 @@ public void equivalentPVs()
100102
assertThat(pvs, hasItem("sim://ramp"));
101103

102104
}
105+
106+
@Test
107+
public void creationAndReleasePVTest() throws Exception {
108+
String disconnectedPVName = "disconnected://missing_PV";
109+
PV disconnectedPV = PVPool.getPV(disconnectedPVName);
110+
int disconnectedRef = 0 ;
111+
String disconnectedRealName = disconnectedPV.getName();
112+
if(!disconnectedRealName.equals(disconnectedPVName)) {
113+
System.out.println("\"" + disconnectedRealName + "\" => PV Name is different = \"" + disconnectedPVName + "\"" );
114+
}
115+
116+
Collection<ReferencedEntry<PV>> pvReferences = PVPool.getPVReferences();
117+
assertThat(pvReferences.size(), equalTo(1));
118+
ReferencedEntry<PV> referenceEntry = PVPool.getReferenceEntry(disconnectedPV);
119+
disconnectedRef = referenceEntry.getReferences();
120+
121+
System.out.println(disconnectedPVName + " nb reference(s) = " + disconnectedRef);
122+
assertThat(disconnectedRef, equalTo(1));
123+
124+
//Create a second PV to check if the same PV is return
125+
disconnectedPV = PVPool.getPV(disconnectedPVName);
126+
127+
//Check if there is 2 references
128+
pvReferences = PVPool.getPVReferences();
129+
assertThat(pvReferences.size(), equalTo(1));
130+
referenceEntry = PVPool.getReferenceEntry(disconnectedPV);
131+
disconnectedRef = referenceEntry.getReferences();
132+
System.out.println(disconnectedPVName + " nb reference(s) = " + disconnectedRef);
133+
assertThat(disconnectedRef, equalTo(2));
134+
135+
//Release PV once to check if there is one reference
136+
PVPool.releasePV(disconnectedPV);
137+
//Check if there is 1 references
138+
referenceEntry = PVPool.getReferenceEntry(disconnectedPV);
139+
disconnectedRef = referenceEntry.getReferences();
140+
System.out.println(disconnectedPVName + " nb reference(s) = " + disconnectedRef);
141+
assertThat(disconnectedRef, equalTo(1));
142+
143+
//Create a second PV that have the same name of Disconnected PV
144+
String testPVName = "test://missing_PV";
145+
int testRef = 0;
146+
PV testPV = PVPool.getPV(testPVName);
147+
148+
String testRealName = testPV.getName();
149+
if(!testRealName.equals(testPVName)) {
150+
System.out.println("\"" + testRealName + "\" => PV Name is different = \"" + testPVName +"\"" );
151+
}
152+
153+
if(testRealName.equals(disconnectedRealName)) {
154+
System.out.println(testPVName + " and " + disconnectedPVName + " have the same name !! =" + testRealName);
155+
}
156+
157+
assertThat(testRealName, equalTo(disconnectedRealName));
158+
159+
//Check if there is 2 references entry
160+
pvReferences = PVPool.getPVReferences();
161+
assertThat(pvReferences.size(), equalTo(2));
162+
163+
referenceEntry = PVPool.getReferenceEntry(disconnectedPV);
164+
disconnectedRef = referenceEntry.getReferences();
165+
System.out.println(disconnectedPVName + " nb reference(s) = " + disconnectedRef);
166+
167+
referenceEntry = PVPool.getReferenceEntry(testPV);
168+
testRef = referenceEntry.getReferences();
169+
System.out.println(testPVName + " nb reference(s) = " + testRef);
170+
171+
assertThat(disconnectedRef, equalTo(1));
172+
assertThat(testRef, equalTo(1));
173+
174+
//Release disconnectedPV
175+
PVPool.releasePV(disconnectedPV);
176+
177+
pvReferences = PVPool.getPVReferences();
178+
assertThat(pvReferences.size(), equalTo(1));
179+
180+
referenceEntry = PVPool.getReferenceEntry(disconnectedPV);
181+
System.out.println(disconnectedPVName + " have no reference");
182+
assertNull(referenceEntry);
183+
184+
//Release testPV
185+
PVPool.releasePV(testPV);
186+
187+
pvReferences = PVPool.getPVReferences();
188+
assertThat(pvReferences.size(), equalTo(0));
189+
190+
referenceEntry = PVPool.getReferenceEntry(testPV);
191+
System.out.println(testPVName + " have no reference");
192+
assertNull(referenceEntry);
193+
}
103194

104195

105196
@Test

‎core/pv/src/test/java/org/phoebus/pv/test/TestPV.java‎

Lines changed: 25 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,25 @@
1+
/*******************************************************************************
2+
* Copyright (c) 2024 aquenos GmbH.
3+
* All rights reserved. This program and the accompanying materials
4+
* are made available under the terms of the Eclipse Public License v1.0
5+
* which accompanies this distribution, and is available at
6+
* http://www.eclipse.org/legal/epl-v10.html
7+
******************************************************************************/
8+
9+
package org.phoebus.pv.test;
10+
11+
import org.epics.vtype.Alarm;
12+
import org.epics.vtype.Time;
13+
import org.epics.vtype.VString;
14+
import org.phoebus.pv.PV;
15+
16+
/**
17+
* Dummy PV implementation that is never going to connect.
18+
*/
19+
public class TestPV extends PV {
20+
21+
protected TestPV(String name) {
22+
super(name);
23+
notifyListenersOfValue(VString.of(name, Alarm.none(), Time.now()));
24+
}
25+
}

‎core/pv/src/test/java/org/phoebus/pv/test/TestPVFactory.java‎

Lines changed: 23 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,23 @@
1+
package org.phoebus.pv.test;
2+
3+
import org.phoebus.pv.PV;
4+
import org.phoebus.pv.PVFactory;
5+
6+
/**
7+
* Creates dummy PVs for use inside tests.
8+
*
9+
* The PVs created do not generate a unique key and will provide the same name of DisconnectedPV
10+
*/
11+
public class TestPVFactory implements PVFactory {
12+
13+
@Override
14+
public String getType() {
15+
return "test";
16+
}
17+
18+
@Override
19+
public PV createPV(String name, String base_name) throws Exception {
20+
return new TestPV(base_name);
21+
}
22+
23+
}

‎core/pv/src/test/resources/META-INF/services/org.phoebus.pv.PVFactory‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1 +1,2 @@
11
org.phoebus.pv.disconnected.DisconnectedPVFactory
2+
org.phoebus.pv.test.TestPVFactory

0 commit comments

Comments
 (0)