Skip to content

FELIX-6815 Fix race condition for defaultBaseUrl #467

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
ghenzler wants to merge 1 commit into
base: master
Choose a base branch
from
+154 −25
Open

FELIX-6815 Fix race condition for defaultBaseUrl #467

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
86 changes: 61 additions & 25 deletions ...heck/generalchecks/src/main/java/org/apache/felix/hc/generalchecks/HttpRequestsCheck.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,7 @@
import java.util.ArrayList;
import java.util.Arrays;
import java.util.Base64;
import java.util.Collections;
import java.util.HashMap;
import java.util.List;
import java.util.Map;
Expand All @@ -58,10 +59,15 @@
import org.apache.felix.hc.generalchecks.util.SimpleConstraintChecker;
import org.apache.felix.utils.json.JSONParser;
import org.osgi.framework.BundleContext;
import org.osgi.framework.Constants;
import org.osgi.framework.InvalidSyntaxException;
import org.osgi.framework.ServiceReference;
import org.osgi.framework.ServiceEvent;
import org.osgi.framework.ServiceListener;
import org.osgi.service.component.annotations.Activate;
import org.osgi.service.component.annotations.Component;
import org.osgi.service.component.annotations.ConfigurationPolicy;
import org.osgi.service.component.annotations.Deactivate;
import org.osgi.service.metatype.annotations.AttributeDefinition;
import org.osgi.service.metatype.annotations.Designate;
import org.osgi.service.metatype.annotations.ObjectClassDefinition;
Expand All @@ -77,6 +83,12 @@ public class HttpRequestsCheck implements HealthCheck {

public static final String HC_NAME = "Http Requests";
public static final String HC_LABEL = "Health Check: " + HC_NAME;
private static final String[] HTTP_SERVICES = {
"org.osgi.service.http.HttpService",
"org.osgi.service.http.runtime.HttpServiceRuntime",
"org.osgi.service.servlet.runtime.HttpServiceRuntime" };
private static final String HTTP_SERVICE_FILTER = "(|(" + Constants.OBJECTCLASS + "=" + String.join(")(" + Constants.OBJECTCLASS + "=", HTTP_SERVICES) + "))";


@ObjectClassDefinition(name = HC_LABEL, description = "Performs http(s) request(s) and checks the response for return code and optionally checks the response entity")
public @interface Config {
Expand Down Expand Up @@ -132,6 +144,7 @@ String[] requests() default {
private final boolean runInParallel;

private volatile String defaultBaseUrl;
private volatile ServiceListener serviceListener;

private FormattingResultLog configErrors = new FormattingResultLog();

Expand All @@ -144,42 +157,59 @@ public HttpRequestsCheck(Config config, BundleContext bundleContext) {
this.statusForFailedContraint = config.statusForFailedContraint();
this.runInParallel = config.runInParallel() && requestSpecs.size() > 1;

LOG.debug("Activated Requests HC: {}", requestSpecs);
this.registerServiceListener();
this.setupDefaultBaseUrl();
LOG.debug("Activated Requests HC: {}, defaultBaseUrl: {}, runInParallel: {}, statusForFailedContraint: {}", requestSpecs, defaultBaseUrl, runInParallel, statusForFailedContraint);
}

@Deactivate
public void deactivate() {
if (this.serviceListener != null) {
this.bundleContext.removeServiceListener(this.serviceListener);
}
}

String getDefaultBaseUrl() {
return defaultBaseUrl;
}

private void registerServiceListener() {
try {
this.serviceListener = new ServiceListener() {
@Override
public void serviceChanged(ServiceEvent event) {
setupDefaultBaseUrl();
LOG.debug("Updated defaultBaseUrl: {}", defaultBaseUrl);
}
};
this.bundleContext.addServiceListener(this.serviceListener, HTTP_SERVICE_FILTER);
} catch (InvalidSyntaxException e) {
LOG.warn("Invalid service filter {}", HTTP_SERVICE_FILTER, e);
}
}

private void setupDefaultBaseUrl() {
// no need to synchronize
if ( this.defaultBaseUrl == null ) {
// check the properties for these services
final String[] services = {"org.osgi.service.http.HttpService",
"org.osgi.service.http.runtime.HttpServiceRuntime",
"org.osgi.service.servlet.runtime.HttpServiceRuntime"};
for(final String service : services) {
final ServiceReference<?> ref = this.bundleContext.getServiceReference(service);
if ( ref != null ) {
boolean isHttp = Boolean.parseBoolean(String.valueOf(ref.getProperty("org.apache.felix.http.enable")));
boolean isHttps = Boolean.parseBoolean(String.valueOf(ref.getProperty("org.apache.felix.https.enable")));
if (isHttps) {
defaultBaseUrl = "http://localhost:"+ref.getProperty("org.osgi.service.https.port");
} else if (isHttp) {
defaultBaseUrl = "http://localhost:"+ref.getProperty("org.osgi.service.http.port");
}
if ( this.defaultBaseUrl != null ) {
break;
}
String resolvedBaseUrl = null;
for (final String service : HTTP_SERVICES) {
final ServiceReference<?> ref = this.bundleContext.getServiceReference(service);
if (ref != null) {
boolean isHttp = Boolean.parseBoolean(String.valueOf(ref.getProperty("org.apache.felix.http.enable")));
boolean isHttps = Boolean.parseBoolean(String.valueOf(ref.getProperty("org.apache.felix.https.enable")));
if (isHttps) {
resolvedBaseUrl = "http://localhost:" + ref.getProperty("org.osgi.service.https.port");
} else if (isHttp) {
resolvedBaseUrl = "http://localhost:" + ref.getProperty("org.osgi.service.http.port");
}
if (resolvedBaseUrl != null) {
break;
}
}
if ( this.defaultBaseUrl == null ) {
this.defaultBaseUrl = "http://localhost:8080";
LOG.debug("Default BaseURL: {}", defaultBaseUrl);
}
}
this.defaultBaseUrl = resolvedBaseUrl;
}

@Override
public Result execute() {
this.setupDefaultBaseUrl();

FormattingResultLog overallLog = new FormattingResultLog();

Expand Down Expand Up @@ -363,6 +393,11 @@ public String toString() {
public FormattingResultLog check(String defaultBaseUrl, int connectTimeoutInMs, int readTimeoutInMs, Result.Status statusForFailedContraint, boolean showTiming) {

FormattingResultLog log = new FormattingResultLog();
if(url.startsWith("/") && (defaultBaseUrl == null || defaultBaseUrl.isEmpty())) {
log.temporarilyUnavailable("Skipping local path {} (HttpService is not available)", url);
return log;
}

String urlWithUser = user!=null ? user + " @ " + url: url;
log.debug("Checking {}", urlWithUser);
log.debug(" configured headers {}", headers.keySet());
Expand Down Expand Up @@ -399,6 +434,7 @@ public Response performRequest(String defaultBaseUrl, String urlWithUser, int co
URL effectiveUrl;
if(url.startsWith("/")) {
effectiveUrl = new URL(defaultBaseUrl + url);
log.debug("Effective URL: {}", effectiveUrl);
} else {
effectiveUrl = new URL(url);
}
Expand Down
93 changes: 93 additions & 0 deletions .../generalchecks/src/test/java/org/apache/felix/hc/generalchecks/HttpRequestsCheckTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,12 @@
import org.apache.felix.hc.generalchecks.HttpRequestsCheck.RequestSpec;
import org.apache.felix.hc.generalchecks.HttpRequestsCheck.ResponseCheck.ResponseCheckResult;
import org.junit.Test;
import org.mockito.ArgumentCaptor;
import org.mockito.Mockito;
import org.osgi.framework.BundleContext;
import org.osgi.framework.ServiceEvent;
import org.osgi.framework.ServiceListener;
import org.osgi.framework.ServiceReference;

public class HttpRequestsCheckTest {

Expand Down Expand Up @@ -175,5 +180,93 @@ public void testSplitArgsRespectingQuotes() throws Exception {
assertArrayEquals(new String[] {"normal1", "\"one two three\"", "normal2", "'one two three'", "-p", "--words", "\"w1 w2 w3\""}, args);
}

@Test
public void testDefaultBaseUrlUpdatesOnServiceEvent() throws Exception {
BundleContext bundleContext = Mockito.mock(BundleContext.class);
ServiceReference httpRef = Mockito.mock(ServiceReference.class);

Mockito.when(bundleContext.getServiceReference("org.osgi.service.http.HttpService"))
.thenReturn((ServiceReference) null, httpRef);
Mockito.when(bundleContext.getServiceReference("org.osgi.service.http.runtime.HttpServiceRuntime")).thenReturn(null);
Mockito.when(bundleContext.getServiceReference("org.osgi.service.servlet.runtime.HttpServiceRuntime")).thenReturn(null);
Mockito.when(httpRef.getProperty("org.apache.felix.http.enable")).thenReturn(true);
Mockito.when(httpRef.getProperty("org.osgi.service.http.port")).thenReturn("9090");

ArgumentCaptor<ServiceListener> listenerCaptor = ArgumentCaptor.forClass(ServiceListener.class);
Mockito.doNothing().when(bundleContext).addServiceListener(listenerCaptor.capture(), anyString());

HttpRequestsCheck httpRequestsCheck = new HttpRequestsCheck(createConfig(), bundleContext);

assertNull(httpRequestsCheck.getDefaultBaseUrl());

listenerCaptor.getValue().serviceChanged(new ServiceEvent(ServiceEvent.REGISTERED, httpRef));

assertEquals("http://localhost:9090", httpRequestsCheck.getDefaultBaseUrl());
}

@Test
public void testRelativeUrlWithoutHttpServiceReturnsUnavailableLog() throws Exception {
HttpRequestsCheck.RequestSpec requestSpec = new HttpRequestsCheck.RequestSpec("/path/to/page.html");
FormattingResultLog resultLog = requestSpec.check(null, 1000, 1000, Result.Status.WARN, false);

Iterator<Entry> entryIt = resultLog.iterator();
Entry lastEntry = null;
while(entryIt.hasNext()) {
lastEntry = entryIt.next();
}

assertEquals(Result.Status.TEMPORARILY_UNAVAILABLE, lastEntry.getStatus());
assertThat(lastEntry.getMessage(), containsString("HttpService is not available"));
}

private HttpRequestsCheck.Config createConfig() {
return new HttpRequestsCheck.Config() {
@Override
public String hc_name() {
return HttpRequestsCheck.HC_NAME;
}

@Override
public String[] hc_tags() {
return new String[0];
}

@Override
public String[] requests() {
return new String[] { "/path/to/page.html" };
}

@Override
public int connectTimeoutInMs() {
return 7000;
}

@Override
public int readTimeoutInMs() {
return 7000;
}

@Override
public Result.Status statusForFailedContraint() {
return Result.Status.WARN;
}

@Override
public boolean runInParallel() {
return false;
}

@Override
public String webconsole_configurationFactory_nameHint() {
return "{hc.name}: {requests}";
}

@Override
public Class<? extends java.lang.annotation.Annotation> annotationType() {
return HttpRequestsCheck.Config.class;
}
};
}


}