BlockHound icon indicating copy to clipboard operation
BlockHound copied to clipboard

BlockHoundIntegration.compareTo doesn't allow integrations loaded via SPI to control ordering based on initial position in stream.

Open johnrengelman opened this issue 3 years ago • 2 comments

The compareTo method is intended to allow integrations to control ordering with respect to other integrations, however the use of Stream.sort() (https://github.com/reactor/BlockHound/blob/master/agent/src/main/java/reactor/blockhound/BlockHound.java#L93) results in the compareTo() method not being called on the first item in the stream. This results in that if an integration is loaded as the first entry, it is unable to change its ordering with respect to other integrations.

Expected Behavior

An integration should be able to fully control its ordering regardless of its initial placement in the stream of detected integrations.

Actual Behavior

The integration that happens to be loaded as the first element of the stream is unable to control it's ordering through implementations of the compareTo method.

Steps to Reproduce

Create an integration while using Netty and call builder.nonBlockingThreadPredicate(Function) in the integration applyTo. In this method try to change the behavior of the thread's non-blocking state. If the integration is loaded first in the chain, it will never be able to override the results from Netty's integration that forces FastLocalThread as a non-blocking thread - https://github.com/netty/netty/blob/4.1/common/src/main/java/io/netty/util/internal/Hidden.java#L144

Possible Solution

Instead of returning a default value of 0 here - https://github.com/reactor/BlockHound/blob/master/agent/src/main/java/reactor/blockhound/integration/BlockHoundIntegration.java#L42, there should be a default method that returns a priority value and the values between the integrations should be compared:

default int getPriority() {
  return 0;
}

@Override
default int compareTo(BlockHoundIntegration o) {
  return getPriority() - o.getPriority();
}

This would allow the integration to impose its ordering regardless of if it is the first or second item in the comparison.

Your Environment

Java 11, BlockHound 1.0.6.RELEASE

johnrengelman avatar Feb 25 '22 18:02 johnrengelman

Cross-posted to Netty as well - https://github.com/netty/netty/issues/12125

johnrengelman avatar Feb 25 '22 20:02 johnrengelman

I can work around this problem by not using the SPI to automatically load the integration and instead call BlockHound.install(new MyIntegration()) to force it to the end of the stream.

johnrengelman avatar Feb 25 '22 20:02 johnrengelman

@johnrengelman , I have applied your suggestion, thanks !

Now, SPI integration plugins can control their ordering by simply overriding the getPriority method.

Note that I'll make a PR in netty in order to suggest to remove the compareToMethod from the Netty BlockHound integration plugin.

pderop avatar Jan 25 '23 09:01 pderop