Skip to content

Add OSGi Component for Dynamic TransactionCountHandler Registration #10

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
RDPerera wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Add OSGi Component for Dynamic TransactionCountHandler Registration #10

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
cb6590b
Add OSGi Component for Dynamic TransactionCountHandler Registration
RDPerera Dec 5, 2025
88d460f
Merge branch 'main' of https://github.com/wso2/usage-data-collector
RDPerera Dec 16, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
188 changes: 188 additions & 0 deletions .../carbon/usage/data/collector/mi/transaction/counter/TransactionCountHandlerComponent.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,188 @@
/*
* Copyright (c) 2025, WSO2 LLC. (http://www.wso2.org) All Rights Reserved.
*
* WSO2 LLC. licenses this file to you 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
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing,
* software distributed under the License is distributed on an
* "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
* KIND, either express or implied. See the License for the
* specific language governing permissions and limitations
* under the License.
*/

package org.wso2.carbon.usage.data.collector.mi.transaction.counter;

import org.apache.commons.logging.Log;
import org.apache.commons.logging.LogFactory;
import org.apache.synapse.AbstractExtendedSynapseHandler;
import org.apache.synapse.MessageContext;
import org.apache.synapse.SynapseHandler;
import org.osgi.service.component.annotations.Activate;
import org.osgi.service.component.annotations.Component;
import org.osgi.service.component.annotations.Deactivate;
import org.osgi.service.component.annotations.Reference;
import org.osgi.service.component.annotations.ReferenceCardinality;
import org.osgi.service.component.annotations.ReferencePolicy;
import org.wso2.carbon.usage.data.collector.mi.transaction.publisher.TransactionPublisher;

/**
* OSGi component that registers TransactionCountHandler as a SynapseHandler service.
* This component will be automatically picked up by DynamicSynapseHandlerRegistrar
* which listens for SynapseHandler services with dynamic cardinality.
*/
@Component(
name = "org.wso2.carbon.usage.data.collector.mi.transaction.counter.handler.component",
service = SynapseHandler.class,
immediate = true,
property = {
"handler.name=TransactionCountHandler",
"handler.enabled=true"
}
)
public class TransactionCountHandlerComponent extends AbstractExtendedSynapseHandler {

private static final Log LOG = LogFactory.getLog(TransactionCountHandlerComponent.class);

private TransactionCountHandler handler;
private TransactionPublisher transactionPublisher;
Comment on lines +52 to +53
Copy link

Copilot AI Dec 5, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The handler and transactionPublisher fields are accessed from multiple threads (OSGi lifecycle methods and SynapseHandler methods) without synchronization. This creates potential race conditions where:

  1. The handler delegation methods could read a null handler while it's being set/unset
  2. The transactionPublisher field could be read/written concurrently during bind/unbind operations

Consider making these fields volatile to ensure visibility across threads, or use proper synchronization mechanisms.

Suggested change
private TransactionCountHandler handler;
private TransactionPublisher transactionPublisher;
private volatile TransactionCountHandler handler;
private volatile TransactionPublisher transactionPublisher;

Copilot uses AI. Check for mistakes.

@Activate
protected void activate() {
try {
LOG.debug("Activating TransactionCountHandler OSGi component");

// Create the handler instance
handler = new TransactionCountHandler();
Copy link

Copilot AI Dec 5, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Creating a new TransactionCountHandler instance in the component's activate method conflicts with the singleton pattern used in TransactionCountHandler itself. The TransactionCountHandler.registerTransactionPublisher() method creates and manages its own singleton instance.

This creates two separate instances:

  1. The instance created here (line 61) stored in the handler field
  2. The static singleton instance managed by TransactionCountHandler

Since the static methods registerTransactionPublisher and unregisterTransactionPublisher operate on the singleton instance, the delegation methods in this component will invoke methods on a different instance than the one being registered with the publisher. This could lead to the handler not functioning correctly.

Consider removing the local instance creation and delegating directly to the singleton instance, or refactoring TransactionCountHandler to not use the singleton pattern.

Copilot uses AI. Check for mistakes.

// Register the publisher if available
if (transactionPublisher != null) {
TransactionCountHandler.registerTransactionPublisher(transactionPublisher);
LOG.debug("TransactionCountHandler registered with TransactionPublisher");
} else {
LOG.warn("TransactionPublisher not available during activation. " +
"Handler will be initialized when publisher becomes available.");
}

LOG.debug("TransactionCountHandler component activated successfully");
} catch (Exception e) {
LOG.error("Error activating TransactionCountHandler component", e);
}
Comment on lines +57 to +75
Copy link

Copilot AI Dec 5, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Catching a generic Exception in lifecycle methods and only logging it can hide critical initialization failures. If the handler fails to initialize properly, the component will appear to be active but won't function correctly.

Consider either:

  1. Letting exceptions propagate to OSGi to mark the component activation as failed
  2. Using a flag to track initialization success and return appropriate values from handler methods when initialization failed
  3. At minimum, using LOG.error() for the activate method failure since it's more critical than deactivation
Suggested change
try {
LOG.debug("Activating TransactionCountHandler OSGi component");
// Create the handler instance
handler = new TransactionCountHandler();
// Register the publisher if available
if (transactionPublisher != null) {
TransactionCountHandler.registerTransactionPublisher(transactionPublisher);
LOG.debug("TransactionCountHandler registered with TransactionPublisher");
} else {
LOG.warn("TransactionPublisher not available during activation. " +
"Handler will be initialized when publisher becomes available.");
}
LOG.debug("TransactionCountHandler component activated successfully");
} catch (Exception e) {
LOG.error("Error activating TransactionCountHandler component", e);
}
LOG.debug("Activating TransactionCountHandler OSGi component");
// Create the handler instance
handler = new TransactionCountHandler();
// Register the publisher if available
if (transactionPublisher != null) {
TransactionCountHandler.registerTransactionPublisher(transactionPublisher);
LOG.debug("TransactionCountHandler registered with TransactionPublisher");
} else {
LOG.warn("TransactionPublisher not available during activation. " +
"Handler will be initialized when publisher becomes available.");
}
LOG.debug("TransactionCountHandler component activated successfully");

Copilot uses AI. Check for mistakes.
}

@Deactivate
protected void deactivate() {
try {
LOG.debug("Deactivating TransactionCountHandler OSGi component");

// Unregister the publisher
if (transactionPublisher != null) {
TransactionCountHandler.unregisterTransactionPublisher(transactionPublisher);
LOG.debug("TransactionCountHandler unregistered from TransactionPublisher");
}

handler = null;
LOG.debug("TransactionCountHandler component deactivated successfully");
} catch (Exception e) {
LOG.error("Error deactivating TransactionCountHandler component", e);
}
}

/**
* Binds the TransactionPublisher service when it becomes available.
*
* @param publisher the TransactionPublisher service
*/
@Reference(
name = "transaction.publisher",
service = TransactionPublisher.class,
cardinality = ReferenceCardinality.OPTIONAL,
policy = ReferencePolicy.DYNAMIC,
unbind = "unsetTransactionPublisher"
)
protected void setTransactionPublisher(TransactionPublisher publisher) {
try {
LOG.debug("TransactionPublisher service bound to TransactionCountHandler component");
this.transactionPublisher = publisher;

// If handler is already created, register it with the publisher
if (handler != null) {
TransactionCountHandler.registerTransactionPublisher(publisher);
LOG.debug("Existing TransactionCountHandler registered with newly bound TransactionPublisher");
}
} catch (Exception e) {
LOG.error("Error binding TransactionPublisher to TransactionCountHandler", e);
}
}
Comment on lines +108 to +121
Copy link

Copilot AI Dec 5, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The setTransactionPublisher method can be called before or after activate() depending on OSGi service availability. This creates a race condition:

  1. If the publisher binds before activation, transactionPublisher is set but the handler is null, so registration is skipped
  2. If the publisher binds after activation, both publisher and handler exist, so registration happens
  3. In the first scenario, the registration that should happen in activate() (line 65) may not actually register with the correct instance due to the singleton issue

Additionally, this method and activate() both call TransactionCountHandler.registerTransactionPublisher() which could lead to multiple registrations or inconsistent state. Consider using a single initialization point or adding guards to ensure registration happens exactly once.

Copilot uses AI. Check for mistakes.

/**
* Unbinds the TransactionPublisher service when it becomes unavailable.
*
* @param publisher the TransactionPublisher service
*/
protected void unsetTransactionPublisher(TransactionPublisher publisher) {
try {
LOG.debug("TransactionPublisher service unbound from TransactionCountHandler component");

if (this.transactionPublisher == publisher) {
TransactionCountHandler.unregisterTransactionPublisher(publisher);
this.transactionPublisher = null;
LOG.debug("TransactionCountHandler unregistered from TransactionPublisher");
}
} catch (Exception e) {
LOG.error("Error unbinding TransactionPublisher from TransactionCountHandler", e);
}
}
Comment on lines +128 to +140
Copy link

Copilot AI Dec 5, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The unsetTransactionPublisher method checks if the publisher being unbound matches the stored reference before unregistering. However, if the publisher service is replaced (not just removed), the old publisher could remain registered in TransactionCountHandler while a new one is bound.

Additionally, the deactivate() method unconditionally unregisters the publisher without checking if it was successfully registered during activation. If activation failed (caught by the try-catch), deactivation will still attempt to unregister, potentially causing issues.

Consider tracking the registration state to ensure symmetric registration/unregistration.

Copilot uses AI. Check for mistakes.

// Delegate all SynapseHandler methods to the actual handler instance

@Override
public boolean handleServerInit() {
return handler != null && handler.handleServerInit();
}

@Override
public boolean handleServerShutDown() {
return handler != null && handler.handleServerShutDown();
}

@Override
public boolean handleRequestInFlow(MessageContext synCtx) {
return handler != null && handler.handleRequestInFlow(synCtx);
}

@Override
public boolean handleRequestOutFlow(MessageContext synCtx) {
return handler != null && handler.handleRequestOutFlow(synCtx);
}

@Override
public boolean handleResponseInFlow(MessageContext synCtx) {
return handler != null && handler.handleResponseInFlow(synCtx);
}

@Override
public boolean handleResponseOutFlow(MessageContext synCtx) {
return handler != null && handler.handleResponseOutFlow(synCtx);
}

@Override
public boolean handleArtifactDeployment(String artifactName, String artifactType, String artifactPath) {
return handler != null && handler.handleArtifactDeployment(artifactName, artifactType, artifactPath);
}

@Override
public boolean handleArtifactUnDeployment(String artifactName, String artifactType, String artifactPath) {
return handler != null && handler.handleArtifactUnDeployment(artifactName, artifactType, artifactPath);
}

@Override
public boolean handleError(MessageContext synCtx) {
return handler != null && handler.handleError(synCtx);
Comment on lines +161 to +186
Copy link

Copilot AI Dec 5, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All SynapseHandler methods return false when the handler is null, which may not be the intended behavior. According to typical SynapseHandler patterns, returning false can indicate that message processing should stop or that the handler failed.

When the handler is not yet initialized (null), it might be more appropriate to return true to indicate successful (no-op) processing and allow the message flow to continue, or to implement different behavior based on the specific method. Consider reviewing the expected behavior for each handler method when the handler is unavailable.

Suggested change
return handler != null && handler.handleRequestOutFlow(synCtx);
}
@Override
public boolean handleResponseInFlow(MessageContext synCtx) {
return handler != null && handler.handleResponseInFlow(synCtx);
}
@Override
public boolean handleResponseOutFlow(MessageContext synCtx) {
return handler != null && handler.handleResponseOutFlow(synCtx);
}
@Override
public boolean handleArtifactDeployment(String artifactName, String artifactType, String artifactPath) {
return handler != null && handler.handleArtifactDeployment(artifactName, artifactType, artifactPath);
}
@Override
public boolean handleArtifactUnDeployment(String artifactName, String artifactType, String artifactPath) {
return handler != null && handler.handleArtifactUnDeployment(artifactName, artifactType, artifactPath);
}
@Override
public boolean handleError(MessageContext synCtx) {
return handler != null && handler.handleError(synCtx);
if (handler != null) {
return handler.handleRequestOutFlow(synCtx);
}
return true;
}
@Override
public boolean handleResponseInFlow(MessageContext synCtx) {
if (handler != null) {
return handler.handleResponseInFlow(synCtx);
}
return true;
}
@Override
public boolean handleResponseOutFlow(MessageContext synCtx) {
if (handler != null) {
return handler.handleResponseOutFlow(synCtx);
}
return true;
}
@Override
public boolean handleArtifactDeployment(String artifactName, String artifactType, String artifactPath) {
if (handler != null) {
return handler.handleArtifactDeployment(artifactName, artifactType, artifactPath);
}
return true;
}
@Override
public boolean handleArtifactUnDeployment(String artifactName, String artifactType, String artifactPath) {
if (handler != null) {
return handler.handleArtifactUnDeployment(artifactName, artifactType, artifactPath);
}
return true;
}
@Override
public boolean handleError(MessageContext synCtx) {
if (handler != null) {
return handler.handleError(synCtx);
}
return true;

Copilot uses AI. Check for mistakes.
}
}