alien11689 commented on code in PR #98:
URL: https://github.com/apache/aries-rsa/pull/98#discussion_r3176360724
##########
rsa/src/main/java/org/apache/aries/rsa/core/ExportRegistrationImpl.java:
##########
@@ -35,167 +38,214 @@
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
+/**
+ * Implements an ExportRegistration. Since there is a 1:1 relationship between
+ * an ExportRegistration and its ExportReference (they are basically two views
+ * of the same underlying data - one is the modifiable part and one the
read-only
+ * part), this class implements both interfaces together.
+ */
@SuppressWarnings("rawtypes")
-public class ExportRegistrationImpl implements ExportRegistration {
+public class ExportRegistrationImpl implements ExportRegistration,
ExportReference {
private static final Logger LOG =
LoggerFactory.getLogger(ExportRegistrationImpl.class);
- private final CloseHandler closeHandler;
- private ExportReferenceImpl exportReference;
- private final Closeable server;
- private final Throwable exception;
+ /**
+ * According to the specs, multiple ExportRegistration can be linked
together,
+ * i.e. they have a shared state - this is implemented via this inner
class,
+ * where every group of linked export registrations reference a single
Shared instance.
+ */
+ private static class Shared {
- private final ExportRegistrationImpl parent;
- private int instanceCount;
- private volatile boolean closed;
+ private Set<CloseHandler> closeHandlers =
Collections.newSetFromMap(new ConcurrentHashMap<>());
+ private EventProducer eventProducer;
+ private ServiceReference serviceReference;
+ private volatile EndpointDescription endpoint;
+ private Closeable server;
+ private Throwable exception;
- private final EventProducer sender;
-
- private ExportRegistrationImpl(ExportRegistrationImpl parent,
- CloseHandler rsaCore,
- EventProducer sender,
- ExportReferenceImpl exportReference,
- Closeable server,
- Throwable exception) {
- this.sender = sender;
- this.parent = parent != null ? parent.parent : this; // a parent
points to itself
- this.parent.addInstance();
- this.closeHandler = rsaCore;
- this.exportReference = exportReference;
- this.server = server;
- this.exception = exception;
- }
+ private int instanceCount;
+
+ void add(CloseHandler closeHandler) {
+ if (closeHandler != null) {
+ closeHandlers.add(closeHandler);
+ }
+ }
+
+ void add() {
+ synchronized (this) {
+ instanceCount++;
+ }
+ }
- // create a clone of the provided ExportRegistrationImpl that is linked to
it
- public ExportRegistrationImpl(ExportRegistrationImpl parent) {
- this(parent, parent.closeHandler, parent.sender, new
ExportReferenceImpl(parent.exportReference),
- parent.server, parent.exception);
+ void remove() {
+ synchronized (this) {
+ if (--instanceCount != 0) {
+ return;
+ }
+ LOG.debug("closing ExportRegistration after removing last
linked instance");
+ if (server != null) {
+ try {
+ server.close();
+ } catch (IOException ioe) {
+ LOG.warn("Error closing ExportRegistration", ioe);
+ }
+ }
+ }
+ }
}
- // create a new (parent) instance which was exported successfully with the
given server
- public ExportRegistrationImpl(ServiceReference sref, Endpoint endpoint,
CloseHandler closeHandler, EventProducer sender) {
- this(null, closeHandler, sender, new ExportReferenceImpl(sref,
endpoint.description()), endpoint, null);
+ // state shared between all linked export registrations in this instance's
group
+ private final Shared shared;
+
+ // per-instance state that is not shared
+ private AtomicBoolean closing = new AtomicBoolean();
+ private volatile boolean closed;
+
+
+ /**
+ * Constructs an export registration that is linked
+ * (shares state) with the given export registration.
+ * <p>
+ * The {@link #close} method must eventually be invoked on this instance.
+ *
+ * @param er the export registration that this instance is linked to
+ */
+ public ExportRegistrationImpl(ExportRegistrationImpl er) {
+ shared = er.shared;
+ shared.add();
}
- // create a new (parent) instance which failed to be exported with the
given exception
- public ExportRegistrationImpl(Throwable exception, CloseHandler
closeHandler, EventProducer sender) {
- this(null, closeHandler, sender, null, null, exception);
+ /**
+ * Constructs a new export registration that is not linked to any other
instance
+ * in a successful state.
+ * <p>
+ * The {@link #close} method must eventually be invoked on this instance.
+ *
+ * @param sref the exported service reference
+ * @param endpoint the exported endpoint info
+ * @param closeHandler a callback function that will be invoked when the
registration is closed
+ * @param eventProducer an event producer that will be invoked when the
endpoint is updated
+ */
+ // create a new (parent) instance which was exported successfully with the
given server
+ public ExportRegistrationImpl(ServiceReference sref, Endpoint endpoint,
+ CloseHandler closeHandler, EventProducer eventProducer) {
+ shared = new Shared();
Review Comment:
let's have a Shared constructor with parameters pased below
##########
rsa/src/main/java/org/apache/aries/rsa/core/ExportRegistrationImpl.java:
##########
@@ -35,167 +38,214 @@
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
+/**
+ * Implements an ExportRegistration. Since there is a 1:1 relationship between
+ * an ExportRegistration and its ExportReference (they are basically two views
+ * of the same underlying data - one is the modifiable part and one the
read-only
+ * part), this class implements both interfaces together.
+ */
@SuppressWarnings("rawtypes")
-public class ExportRegistrationImpl implements ExportRegistration {
+public class ExportRegistrationImpl implements ExportRegistration,
ExportReference {
private static final Logger LOG =
LoggerFactory.getLogger(ExportRegistrationImpl.class);
- private final CloseHandler closeHandler;
- private ExportReferenceImpl exportReference;
- private final Closeable server;
- private final Throwable exception;
+ /**
+ * According to the specs, multiple ExportRegistration can be linked
together,
+ * i.e. they have a shared state - this is implemented via this inner
class,
+ * where every group of linked export registrations reference a single
Shared instance.
+ */
+ private static class Shared {
- private final ExportRegistrationImpl parent;
- private int instanceCount;
- private volatile boolean closed;
+ private Set<CloseHandler> closeHandlers =
Collections.newSetFromMap(new ConcurrentHashMap<>());
+ private EventProducer eventProducer;
+ private ServiceReference serviceReference;
+ private volatile EndpointDescription endpoint;
+ private Closeable server;
+ private Throwable exception;
- private final EventProducer sender;
-
- private ExportRegistrationImpl(ExportRegistrationImpl parent,
- CloseHandler rsaCore,
- EventProducer sender,
- ExportReferenceImpl exportReference,
- Closeable server,
- Throwable exception) {
- this.sender = sender;
- this.parent = parent != null ? parent.parent : this; // a parent
points to itself
- this.parent.addInstance();
- this.closeHandler = rsaCore;
- this.exportReference = exportReference;
- this.server = server;
- this.exception = exception;
- }
+ private int instanceCount;
+
+ void add(CloseHandler closeHandler) {
+ if (closeHandler != null) {
+ closeHandlers.add(closeHandler);
+ }
+ }
+
+ void add() {
+ synchronized (this) {
+ instanceCount++;
+ }
+ }
- // create a clone of the provided ExportRegistrationImpl that is linked to
it
- public ExportRegistrationImpl(ExportRegistrationImpl parent) {
- this(parent, parent.closeHandler, parent.sender, new
ExportReferenceImpl(parent.exportReference),
- parent.server, parent.exception);
+ void remove() {
+ synchronized (this) {
+ if (--instanceCount != 0) {
+ return;
+ }
+ LOG.debug("closing ExportRegistration after removing last
linked instance");
+ if (server != null) {
+ try {
+ server.close();
+ } catch (IOException ioe) {
+ LOG.warn("Error closing ExportRegistration", ioe);
+ }
+ }
+ }
+ }
}
- // create a new (parent) instance which was exported successfully with the
given server
- public ExportRegistrationImpl(ServiceReference sref, Endpoint endpoint,
CloseHandler closeHandler, EventProducer sender) {
- this(null, closeHandler, sender, new ExportReferenceImpl(sref,
endpoint.description()), endpoint, null);
+ // state shared between all linked export registrations in this instance's
group
+ private final Shared shared;
+
+ // per-instance state that is not shared
+ private AtomicBoolean closing = new AtomicBoolean();
+ private volatile boolean closed;
+
+
+ /**
+ * Constructs an export registration that is linked
+ * (shares state) with the given export registration.
+ * <p>
+ * The {@link #close} method must eventually be invoked on this instance.
+ *
+ * @param er the export registration that this instance is linked to
+ */
+ public ExportRegistrationImpl(ExportRegistrationImpl er) {
+ shared = er.shared;
+ shared.add();
}
- // create a new (parent) instance which failed to be exported with the
given exception
- public ExportRegistrationImpl(Throwable exception, CloseHandler
closeHandler, EventProducer sender) {
- this(null, closeHandler, sender, null, null, exception);
+ /**
+ * Constructs a new export registration that is not linked to any other
instance
+ * in a successful state.
+ * <p>
+ * The {@link #close} method must eventually be invoked on this instance.
+ *
+ * @param sref the exported service reference
+ * @param endpoint the exported endpoint info
+ * @param closeHandler a callback function that will be invoked when the
registration is closed
+ * @param eventProducer an event producer that will be invoked when the
endpoint is updated
+ */
+ // create a new (parent) instance which was exported successfully with the
given server
+ public ExportRegistrationImpl(ServiceReference sref, Endpoint endpoint,
+ CloseHandler closeHandler, EventProducer eventProducer) {
+ shared = new Shared();
+ shared.eventProducer = eventProducer;
+ shared.serviceReference = sref;
+ shared.endpoint = endpoint.description();
+ shared.server = endpoint;
+ addCloseHandler(closeHandler);
+ shared.add();
}
- private void ensureParent() {
- if (parent != this) {
- throw new IllegalStateException("this method may only be called on
the parent");
- }
+ /**
+ * Constructs a new export registration that is not linked to any other
instance
+ * in a failed state.
+ * <p>
+ * The {@link #close} method must eventually be invoked on this instance.
+ *
+ * @param exception the exception that occurred during initialization
+ * @param closeHandler a callback function that will be invoked when the
registration is closed
+ * @param eventProducer an event producer that will be invoked when the
endpoint is updated
+ */
+ public ExportRegistrationImpl(Throwable exception, CloseHandler
closeHandler, EventProducer eventProducer) {
+ shared = new Shared();
Review Comment:
can we have an explicit constructor (of static factory method in Shared)?
##########
rsa/src/main/java/org/apache/aries/rsa/core/ExportRegistrationImpl.java:
##########
@@ -35,167 +38,214 @@
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
+/**
+ * Implements an ExportRegistration. Since there is a 1:1 relationship between
+ * an ExportRegistration and its ExportReference (they are basically two views
+ * of the same underlying data - one is the modifiable part and one the
read-only
+ * part), this class implements both interfaces together.
+ */
@SuppressWarnings("rawtypes")
-public class ExportRegistrationImpl implements ExportRegistration {
+public class ExportRegistrationImpl implements ExportRegistration,
ExportReference {
private static final Logger LOG =
LoggerFactory.getLogger(ExportRegistrationImpl.class);
- private final CloseHandler closeHandler;
- private ExportReferenceImpl exportReference;
- private final Closeable server;
- private final Throwable exception;
+ /**
+ * According to the specs, multiple ExportRegistration can be linked
together,
+ * i.e. they have a shared state - this is implemented via this inner
class,
+ * where every group of linked export registrations reference a single
Shared instance.
+ */
+ private static class Shared {
- private final ExportRegistrationImpl parent;
- private int instanceCount;
- private volatile boolean closed;
+ private Set<CloseHandler> closeHandlers =
Collections.newSetFromMap(new ConcurrentHashMap<>());
+ private EventProducer eventProducer;
+ private ServiceReference serviceReference;
+ private volatile EndpointDescription endpoint;
+ private Closeable server;
+ private Throwable exception;
- private final EventProducer sender;
-
- private ExportRegistrationImpl(ExportRegistrationImpl parent,
- CloseHandler rsaCore,
- EventProducer sender,
- ExportReferenceImpl exportReference,
- Closeable server,
- Throwable exception) {
- this.sender = sender;
- this.parent = parent != null ? parent.parent : this; // a parent
points to itself
- this.parent.addInstance();
- this.closeHandler = rsaCore;
- this.exportReference = exportReference;
- this.server = server;
- this.exception = exception;
- }
+ private int instanceCount;
+
+ void add(CloseHandler closeHandler) {
+ if (closeHandler != null) {
+ closeHandlers.add(closeHandler);
+ }
+ }
+
+ void add() {
Review Comment:
add above has completely different behaviour than this one - can we rename
`add` and `remove` method to somehting more meaningful e.g. `registerInstance`
and `unregisterInstance`?
##########
rsa/src/main/java/org/apache/aries/rsa/core/ImportRegistrationImpl.java:
##########
@@ -32,205 +31,238 @@
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
+/**
+ * Implements an ImportRegistration. Since there is a 1:1 relationship between
+ * an ImportRegistration and its ImportReference (they are basically two views
+ * of the same underlying data - one is the modifiable part and one the
read-only
+ * part), this class implements both interfaces together.
+ */
@SuppressWarnings("rawtypes")
public class ImportRegistrationImpl implements ImportRegistration,
ImportReference {
private static final Logger LOG =
LoggerFactory.getLogger(ImportRegistrationImpl.class);
- private volatile Throwable exception;
- private volatile ServiceRegistration importedService; // used only in
parent
- private EndpointDescription endpoint;
- private volatile ClientServiceFactory clientServiceFactory;
- private CloseHandler closeHandler;
- private AtomicBoolean closed = new AtomicBoolean();
- private AtomicBoolean closing = new AtomicBoolean();
- private boolean detached; // used only in parent
+ /**
+ * According to the specs, multiple ImportRegistrations can be linked
together,
+ * i.e. they have a shared state - this is implemented via this inner
class,
+ * where every group of linked import registrations reference a single
Shared instance.
+ */
+ private static class Shared {
+
+ private Set<CloseHandler> closeHandlers =
Collections.newSetFromMap(new ConcurrentHashMap<>());
+ private EventProducer eventProducer;
+ private volatile EndpointDescription endpoint;
+ private volatile Throwable exception;
+ private volatile ClientServiceFactory clientServiceFactory;
+ private volatile ServiceRegistration importedService;
+ // all linked import registrations that share this state
+ private final List<ImportRegistrationImpl> instances = new
ArrayList<>(1);
+
+ void add(CloseHandler closeHandler) {
+ if (closeHandler != null) {
+ closeHandlers.add(closeHandler);
+ }
+ }
- private ImportRegistrationImpl parent;
- private List<ImportRegistrationImpl> children; // used only in parent
+ /**
+ * Add a linked ImportRegistration instance to the shared data.
+ *
+ * @param ir the instance to add
+ */
+ private synchronized void add(ImportRegistrationImpl ir) {
+ instances.add(ir);
+ }
- private EventProducer eventProducer;
+ /**
+ * Remove a linked ImportRegistration from the shared data.
+ *
+ * @param ir the instance to remove
+ */
+ private void remove(ImportRegistrationImpl ir) {
+ // close the underlying service only once on the last remove (not
before or after)
+ synchronized (this) {
+ boolean removed = instances.remove(ir);
+ if (!removed || !instances.isEmpty()) {
+ return;
+ }
+ }
+
+ LOG.debug("closing ImportRegistration after removing last linked
instance");
+ if (importedService != null) {
+ try {
+ importedService.unregister();
+ } catch (IllegalStateException ise) {
+ LOG.debug("imported service is already unregistered");
+ }
+ importedService = null;
+ }
+ if (clientServiceFactory != null) {
+ clientServiceFactory.setCloseable(true);
+ }
+ }
- public ImportRegistrationImpl(Throwable ex) {
- exception = ex;
- initParent();
+ void closeAll() {
+ // make a copy to avoid ConcurrentModificationException
+ // when instances are removed during iteration
+ List<ImportRegistrationImpl> copy;
+ synchronized (this) {
+ copy = new ArrayList<>(instances);
+ }
+ LOG.info("closing all linked ImportRegistrations");
+ for (ImportRegistrationImpl ir : copy) {
+ ir.close();
+ }
+ }
}
+ // state shared between all linked import registrations in this instance's
group
+ private Shared shared;
+
+ // per-instance state that is not shared
+ private AtomicBoolean closing = new AtomicBoolean();
+ private volatile boolean closed;
+
+ /**
+ * Constructs a new import registration that is not linked to any other
instance.
+ * <p>
+ * The {@link #close} method must eventually be invoked on this instance.
+ *
+ * @param endpoint the imported endpoint info
+ * @param closeHandler a callback function that will be invoked when the
registration is closed
+ * @param eventProducer an event producer that will be invoked when the
endpoint is updated
+ */
public ImportRegistrationImpl(EndpointDescription endpoint, CloseHandler
closeHandler, EventProducer eventProducer) {
- this.endpoint = endpoint;
- this.closeHandler = closeHandler;
- this.eventProducer = eventProducer;
- initParent();
+ shared = new Shared();
+ shared.endpoint = endpoint;
+ shared.eventProducer = eventProducer;
+ addCloseHandler(closeHandler);
+ shared.add(this);
}
/**
- * Creates a clone of the given parent instance.
+ * Constructs an import registration that is linked
+ * (shares state) with the given import registration.
+ * <p>
+ * The {@link #close} method must eventually be invoked on this instance.
+ *
+ * @param ir the import registration that this instance is linked to
*/
public ImportRegistrationImpl(ImportRegistration ir) {
- // we always want a link to the parent...
- parent = ((ImportRegistrationImpl)ir).getParent();
- exception = parent.getException();
- endpoint = parent.getImportedEndpointDescription();
- clientServiceFactory = parent.clientServiceFactory;
- closeHandler = parent.closeHandler;
-
- parent.instanceAdded(this);
- }
-
- private void initParent() {
- parent = this;
- children = new ArrayList<>(1);
+ shared = ((ImportRegistrationImpl)ir).shared;
+ shared.add(this);
}
- private void ensureParent() {
- if (parent != this) {
- throw new IllegalStateException("this method may only be called on
the parent");
+ private void init(ClientServiceFactory csf, ServiceRegistration sreg,
Throwable exception) {
+ if (isInvalid() || shared.clientServiceFactory != null ||
shared.importedService != null) {
+ throw new IllegalStateException("already initialized");
}
+ shared.clientServiceFactory = csf;
+ shared.importedService = sreg;
+ shared.exception = exception;
}
/**
- * Called on parent when a child is added.
+ * Initializes this import registration in an error state.
*
- * @param iri the child
+ * @param exception the exception that occurred during initialization
*/
- private synchronized void instanceAdded(ImportRegistrationImpl iri) {
- ensureParent();
- children.add(iri);
+ public void init(Throwable exception) {
+ init(null, null, exception);
}
/**
- * Called on parent when a child is closed.
+ * Initializes this import registration in a successful state.
*
- * @param iri the child
+ * @param csf the {@link ClientServiceFactory} which is the implementation
+ * of the locally registered service which provides proxies to
the
+ * remote imported service.
+ * @param sreg the {@link ServiceRegistration} representing the locally
+ * registered {@link ClientServiceFactory} service which
provides
+ * proxies to the remote imported service.
*/
- private void instanceClosed(ImportRegistration iri) {
- ensureParent();
- synchronized (this) {
- children.remove(iri);
- if (!children.isEmpty() || detached || !closing.get()) {
- return;
- }
- detached = true;
- }
+ public void init(ClientServiceFactory csf, ServiceRegistration sreg) {
+ init(csf, sreg, null);
+ }
- LOG.debug("really closing ImportRegistration now");
+ /**
+ * Returns whether this registration is invalid
+ * (has an initialization exception) or closed.
+ *
+ * @return whether this registration is invalid or closed
+ */
+ private boolean isInvalid() {
+ return shared.exception != null || closed;
+ }
- if (importedService != null) {
- try {
- importedService.unregister();
- } catch (IllegalStateException ise) {
- LOG.debug("imported service is already unregistered");
- }
- importedService = null;
- }
- if (clientServiceFactory != null) {
- clientServiceFactory.setCloseable(true);
- }
+ public void addCloseHandler(CloseHandler closeHandler) {
+ shared.add(closeHandler);
}
+ /**
+ * Closes this instance.
+ * <p>
+ * If this is the last instance in its group of linked instances,
+ * the underlying import registration is also closed, otherwise
+ * the import itself remains open for use by the other instances.
+ */
+ @Override
public void close() {
- LOG.debug("close() called");
- synchronized (this) {
- boolean curClosing = closing.getAndSet(true);
- if (curClosing || isInvalid()) {
- return;
- }
+ // we do this in two steps: first the 'closing' state to make sure we
+ // proceed at most once (only if valid and not already closing/closed),
+ // with all the references still valid while calling the close handler,
+ // and only after it's done we reach the 'closed' state where all
+ // getters return null (as required by the spec)
+ LOG.debug("closing import registration");
+ if (closed || closing.getAndSet(true)) {
+ return;
}
- closeHandler.onClose(this);
- parent.instanceClosed(this);
- closed.set(true);
+ shared.closeHandlers.forEach(h -> h.onClose(this));
+ shared.remove(this);
+ closed = true;
}
/**
- * Closes all ImportRegistrations which share the same parent as this one.
+ * Closes this instance, all linked instances, and the underlying service.
*/
public void closeAll() {
- if (this == parent) {
- LOG.info("closing down all child ImportRegistrations");
-
- // we must iterate over a copy of children since close() removes
the child
- // from the list (which would cause a
ConcurrentModificationException)
- for (ImportRegistrationImpl ir : copyChildren()) {
- ir.close();
- }
- this.close();
- } else {
- parent.closeAll();
- }
+ shared.closeAll(); // invokes close() on all shared instances
(including this one)
}
- private List<ImportRegistrationImpl> copyChildren() {
- synchronized (this) {
- return new ArrayList<>(children);
+ @Override
+ @SuppressWarnings("unchecked")
+ public boolean update(EndpointDescription endpoint) {
+ if (isInvalid()) {
+ throw new IllegalStateException("import registration is invalid or
closed");
}
- }
-
- public EndpointDescription getImportedEndpointDescription() {
- return isInvalid() ? null : endpoint;
+ if (!endpoint.getId().equals(shared.endpoint.getId())) {
+ throw new IllegalArgumentException("wrong endpoint id " +
endpoint.getId());
+ }
+ shared.endpoint = endpoint;
+ shared.importedService.setProperties(new
Hashtable<>(endpoint.getProperties()));
+ shared.eventProducer.notifyUpdate(this);
Review Comment:
honestly I am a big fan of OOP and reaching the fields without accessors or
calling explicit methods seems odd to me
--
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]
