Skip to content
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.

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

Fix app failure under Eclipse OSGi #2521

Merged
merged 3 commits into from
Mar 10, 2021
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
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
Original file line number Diff line number Diff line change
@@ -0,0 +1,22 @@
apply from: "$rootDir/gradle/instrumentation.gradle"

// this instrumentation applies to the class 'org.eclipse.osgi.internal.loader.BundleLoader'
// which is present in the following artifacts dating back to version 3.6 (2010):
//
// * 'org.eclipse.platform:org.eclipse.osgi'
// * 'org.eclipse.tycho:org.eclipse.osgi'
// * 'org.eclipse.osgi:org.eclipse.osgi'

// TODO write a smoke test that does the following:
//
// docker run --mount 'type=bind,src=$AGENT_PATH,dst=/opentelemetry-javaagent-all.jar'
// -e JAVA_TOOL_OPTIONS=-javaagent:/opentelemetry-javaagent-all.jar
// wso2/wso2ei-business-process:6.5.0
//
// without this instrumentation, the following error will appear in the docker logs:
// java.lang.ClassNotFoundException: org.wso2.carbon.humantask.ui.fileupload.HumanTaskUploadExecutor
// cannot be found by org.wso2.carbon.ui_4.4.36
//
// ... or even better, write a standalone OSGi application that exhibits similar issue,
// so we can run against arbitrary (e.g. latest) Eclipse OSGi release, especially since
// this instrumentation patches a private method which could be renamed at any time
Original file line number Diff line number Diff line change
@@ -0,0 +1,78 @@
/*
* Copyright The OpenTelemetry Authors
* SPDX-License-Identifier: Apache-2.0
*/

package io.opentelemetry.javaagent.instrumentation.osgi;

import static java.util.Collections.singletonList;
import static java.util.Collections.singletonMap;
import static net.bytebuddy.matcher.ElementMatchers.isMethod;
import static net.bytebuddy.matcher.ElementMatchers.named;
import static net.bytebuddy.matcher.ElementMatchers.returns;

import com.google.auto.service.AutoService;
import io.opentelemetry.javaagent.instrumentation.api.internal.InClassLoaderMatcher;
import io.opentelemetry.javaagent.tooling.InstrumentationModule;
import io.opentelemetry.javaagent.tooling.TypeInstrumentation;
import java.util.List;
import java.util.Map;
import net.bytebuddy.asm.Advice;
import net.bytebuddy.description.method.MethodDescription;
import net.bytebuddy.description.type.TypeDescription;
import net.bytebuddy.matcher.ElementMatcher;

/**
* The ClassLoaderMatcher's call to ClassLoader.getResource() causes the Eclipse OSGi class loader
* to "dynamically import" a bundle for the package if such a bundle is not found, which can lead to
* application failure later on due to the bundle hierarchy no longer being "consistent".
*
* <p>Any side-effect of the ClassLoaderMatcher's call to ClassLoader.getResource() is generally
* undesirable, and so this instrumentation patches the behavior and suppresses the "dynamic import"
* of the missing package/bundle when the call is originating from ClassLoaderMatcher..
*/
@AutoService(InstrumentationModule.class)
public class EclipseOsgiInstrumentationModule extends InstrumentationModule {
public EclipseOsgiInstrumentationModule() {
super("eclipse-osgi");
}

@Override
public List<TypeInstrumentation> typeInstrumentations() {
return singletonList(new EclipseOsgiInstrumentation());
}

private static class EclipseOsgiInstrumentation implements TypeInstrumentation {

@Override
public ElementMatcher<TypeDescription> typeMatcher() {
return named("org.eclipse.osgi.internal.loader.BundleLoader");
}

@Override
public Map<? extends ElementMatcher<? super MethodDescription>, String> transformers() {
return singletonMap(
isMethod().and(named("isDynamicallyImported")).and(returns(boolean.class)),
EclipseOsgiInstrumentation.class.getName() + "$IsDynamicallyImportedAdvice");
}

public static class IsDynamicallyImportedAdvice {

// "skipOn" is used to skip execution of the instrumented method when a ClassLoaderMatcher is
// currently executing, since we will be returning false regardless in onExit below
@Advice.OnMethodEnter(skipOn = Advice.OnNonDefaultValue.class, suppress = Throwable.class)
public static boolean onEnter() {
return InClassLoaderMatcher.get();
}

@Advice.OnMethodExit(onThrowable = Throwable.class, suppress = Throwable.class)
public static void onExit(
@Advice.Return(readOnly = false) boolean result,
@Advice.Enter boolean inClassLoaderMatcher) {
if (inClassLoaderMatcher) {
trask marked this conversation as resolved.
Show resolved Hide resolved
result = false;
}
}
}
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,61 @@
/*
* Copyright The OpenTelemetry Authors
* SPDX-License-Identifier: Apache-2.0
*/

package io.opentelemetry.javaagent.instrumentation.api.internal;

public final class InClassLoaderMatcher {

private static final ThreadLocal<MutableBoolean> inClassLoaderMatcher =
ThreadLocal.withInitial(MutableBoolean::new);

private InClassLoaderMatcher() {}

/**
* Returns whether the ClassLoaderMatcher is currently executing.
*
* <p>This is used (at least) by the {@code eclipse-osgi} instrumentation in order to suppress a
* side effect in the Eclipse OSGi class loader that occurs when ClassLoaderMatcher calls
* ClassLoader.getResource(). See {@code EclipseOsgiInstrumentationModule} for more details.
*/
public static boolean get() {
return inClassLoaderMatcher.get().value;
}

/**
* WARNING This should not be used by instrumentation. It should only be used by
* io.opentelemetry.javaagent.tooling.bytebuddy.matcher.ClassLoaderMatcher.
*
* <p>The reason it can't be (easily) hidden is that this class needs to live in the bootstrap
* class loader to be accessible to instrumentation, while the ClassLoaderMatcher lives in the
* agent class loader.
*/
public static boolean getAndSet(boolean value) {
return inClassLoaderMatcher.get().getAndSet(value);
}

/**
* WARNING This should not be used by instrumentation. It should only be used by
* io.opentelemetry.javaagent.tooling.bytebuddy.matcher.ClassLoaderMatcher.
*
* <p>The reason it can't be (easily) hidden is that this class needs to live in the bootstrap
* class loader to be accessible to instrumentation, while the ClassLoaderMatcher lives in the
* agent class loader.
*/
public static void set(boolean value) {
inClassLoaderMatcher.get().value = value;
}

// using MutableBoolean to avoid an extra thread local lookup for getAndSet()
private static class MutableBoolean {

private boolean value;

private boolean getAndSet(boolean value) {
boolean oldValue = this.value;
this.value = value;
return oldValue;
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
package io.opentelemetry.javaagent.tooling.bytebuddy.matcher;

import io.opentelemetry.javaagent.bootstrap.WeakCache;
import io.opentelemetry.javaagent.instrumentation.api.internal.InClassLoaderMatcher;
import io.opentelemetry.javaagent.tooling.AgentTooling;
import net.bytebuddy.matcher.ElementMatcher;

Expand Down Expand Up @@ -45,10 +46,15 @@ private ClassLoaderHasClassesNamedMatcher(String... classNames) {
}

private boolean hasResources(ClassLoader cl) {
for (String resource : resources) {
if (cl.getResource(resource) == null) {
return false;
boolean priorValue = InClassLoaderMatcher.getAndSet(true);
try {
for (String resource : resources) {
if (cl.getResource(resource) == null) {
return false;
}
}
} finally {
InClassLoaderMatcher.set(priorValue);
}
return true;
}
Expand Down
1 change: 1 addition & 0 deletions settings.gradle
Original file line number Diff line number Diff line change
Expand Up @@ -191,6 +191,7 @@ include ':instrumentation:opentelemetry-api-1.0:javaagent'
include ':instrumentation:opentelemetry-api-metrics-1.0:javaagent'
include ':instrumentation:oshi:javaagent'
include ':instrumentation:oshi:library'
include ':instrumentation:eclipse-osgi-3.6:javaagent'
include ':instrumentation:play:play-2.3:javaagent'
include ':instrumentation:play:play-2.4:javaagent'
include ':instrumentation:play:play-2.6:javaagent'
Expand Down