From 971ad30bfe88c1e5040f27c18606c5d9c0417df6 Mon Sep 17 00:00:00 2001 From: Robert von Burg Date: Thu, 8 Feb 2024 13:08:55 +0100 Subject: [PATCH] [Fix] Fixed race conditions in DefaultRuntimeRegistry, removed Registry.exists(String, Class) method the exists(String, Class) method could be misleading, as an IO with the given ID might exist, but has a different type. This would not make adding an IO with the given ID possible and would lead to exceptions later on. --- .../main/java/com/pi4j/registry/Registry.java | 9 -- .../pi4j/registry/impl/DefaultRegistry.java | 6 - .../registry/impl/DefaultRuntimeRegistry.java | 110 ++++++------------ 3 files changed, 36 insertions(+), 89 deletions(-) diff --git a/pi4j-core/src/main/java/com/pi4j/registry/Registry.java b/pi4j-core/src/main/java/com/pi4j/registry/Registry.java index adf6cf08..f11d5111 100644 --- a/pi4j-core/src/main/java/com/pi4j/registry/Registry.java +++ b/pi4j-core/src/main/java/com/pi4j/registry/Registry.java @@ -47,15 +47,6 @@ * @version $Id: $Id */ public interface Registry extends Describable { - - /** - *

exists.

- * - * @param id a {@link java.lang.String} object. - * @param type a {@link java.lang.Class} object. - * @return a boolean. - */ - boolean exists(String id, Class type); /** *

exists.

* diff --git a/pi4j-core/src/main/java/com/pi4j/registry/impl/DefaultRegistry.java b/pi4j-core/src/main/java/com/pi4j/registry/impl/DefaultRegistry.java index 6b831c90..92e45361 100644 --- a/pi4j-core/src/main/java/com/pi4j/registry/impl/DefaultRegistry.java +++ b/pi4j-core/src/main/java/com/pi4j/registry/impl/DefaultRegistry.java @@ -63,12 +63,6 @@ private DefaultRegistry(RuntimeRegistry registry) { this.registry = registry; } - /** {@inheritDoc} */ - @Override - public boolean exists(String id, Class type) { - return registry.exists(id, type); - } - /** {@inheritDoc} */ @Override public boolean exists(String id) { diff --git a/pi4j-core/src/main/java/com/pi4j/registry/impl/DefaultRuntimeRegistry.java b/pi4j-core/src/main/java/com/pi4j/registry/impl/DefaultRuntimeRegistry.java index 8b3c7323..4abd5af7 100644 --- a/pi4j-core/src/main/java/com/pi4j/registry/impl/DefaultRuntimeRegistry.java +++ b/pi4j-core/src/main/java/com/pi4j/registry/impl/DefaultRuntimeRegistry.java @@ -10,7 +10,7 @@ * This file is part of the Pi4J project. More information about * this project can be found here: https://pi4j.com/ * ********************************************************************** - * + * * Licensed 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 @@ -45,9 +45,9 @@ */ public class DefaultRuntimeRegistry implements RuntimeRegistry { - private Logger logger = LoggerFactory.getLogger(this.getClass()); - private Runtime runtime = null; - private Map instances = new ConcurrentHashMap<>(); + private static final Logger logger = LoggerFactory.getLogger(DefaultRuntimeRegistry.class); + private final Runtime runtime; + private final Map instances; // static singleton instance /** @@ -62,25 +62,25 @@ public static RuntimeRegistry newInstance(Runtime runtime){ // private constructor private DefaultRuntimeRegistry(Runtime runtime) { - // forbid object construction - // set local runtime reference + this.instances = new ConcurrentHashMap<>(); this.runtime = runtime; - } + } - /** {@inheritDoc} */ @Override public RuntimeRegistry add(IO instance) throws IOInvalidIDException, IOAlreadyExistsException { - // validate target I/O instance id String _id = validateId(instance.id()); // first test to make sure this id does not already exist in the registry - if(instances.containsKey(_id)) - throw new IOAlreadyExistsException(_id); + synchronized (this.instances) { + if (instances.containsKey(_id)) + throw new IOAlreadyExistsException(_id); + + // add instance to collection + instances.put(_id, instance); + } - // add instance to collection - instances.put(_id, instance); return this; } @@ -88,65 +88,50 @@ public RuntimeRegistry add(IO instance) throws IOInvalidIDException, IOAlreadyEx @Override public T get(String id, Class type) throws IOInvalidIDException, IONotFoundException { String _id = validateId(id); - - // first test to make sure this id is included in the registry - if(!instances.containsKey(_id)) + T t = (T) instances.get(_id); + if(t == null) throw new IONotFoundException(_id); - - return (T)instances.get(_id); + return t; } /** {@inheritDoc} */ @Override public T get(String id) throws IOInvalidIDException, IONotFoundException { String _id = validateId(id); - - // first test to make sure this id is included in the registry - if(!instances.containsKey(_id)) + T t = (T) instances.get(_id); + if(t == null) throw new IONotFoundException(_id); - - return (T)instances.get(_id); + return t; } /** {@inheritDoc} */ @Override public T remove(String id) throws IONotFoundException, IOInvalidIDException, IOShutdownException { - String _id = validateId(id); - IO shutdownInstance = null; - - // first test to make sure this id is included in the registry - if(!exists(_id)) - throw new IONotFoundException(_id); + synchronized (this.instances) { + String _id = validateId(id); - // shutdown instance - try { - shutdownInstance = instances.get(_id); - shutdownInstance.shutdown(runtime.context()); - } - catch (LifecycleException e){ - logger.error(e.getMessage(), e); - throw new IOShutdownException(shutdownInstance, e); - } + // remove the instance from the registry + IO io = this.instances.remove(_id); + if (io == null) + throw new IONotFoundException(_id); - // remove the shutdown instance from the registry - this.instances.remove(_id); + // shutdown instance + try { + io.shutdown(runtime.context()); + } catch (LifecycleException e) { + logger.error(e.getMessage(), e); + throw new IOShutdownException(io, e); + } - // return the shutdown I/O provider instances - return (T)shutdownInstance; + // return the shutdown I/O provider instances + return (T) io; + } } /** {@inheritDoc} */ @Override public boolean exists(String id) { - String _id = null; - try { - _id = validateId(id); - // return 'false' if the requested ID is not found - // return 'true' if the requested ID is found - return instances.containsKey(_id); - } catch (IOInvalidIDException e) { - return false; - } + return instances.containsKey(validateId(id)); } /** {@inheritDoc} */ @@ -155,28 +140,6 @@ public boolean exists(String id) { return Collections.unmodifiableMap(this.instances); } - /** {@inheritDoc} */ - @Override - public boolean exists(String id, Class type){ - String _id = null; - try { - _id = validateId(id); - - // return 'false' if the requested ID is not found - if(!instances.containsKey(_id)) - return false; - - // get the I/O instance - IO instance = instances.get(id); - - // return true if the I/O instance matches the requested I/O type - return type.isAssignableFrom(instance.getClass()); - } catch (IOInvalidIDException e) { - return false; - } - } - - private String validateId(String id) throws IOInvalidIDException { if(id == null) throw new IOInvalidIDException(); @@ -205,5 +168,4 @@ public RuntimeRegistry initialize() throws InitializeException { // NOTHING TO INITIALIZE return this; } - }