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

BlockhoundIntegration SPI plugins ordering #327

Merged
merged 2 commits into from
Jan 25, 2023

Conversation

pderop
Copy link
Contributor

@pderop pderop commented Jan 24, 2023

The BlockHound.loadIntegration() method is intended to control ordering of BlockhoundIntegration.applyTo calls with respect to other integrations plugins. So, loadIntegrations() first loads all SPI plugins from META-INF/services, then appends any optional plugins provided in the loadIntegration(...) arguments, and finally sort the list before invoking the plugins applyTo methods.

However, by default, the BlockhoundIntegration.compareTo is implemented like this:

default int compareTo(BlockHoundIntegration o) {
    return 0;
}

So, let's say we have these integration plugins being loaded from META-INF/services/... using the BlockHound.loadIntegrations() method:

    public class I1 implements BlockHoundIntegration {
        public void applyTo(BlockHound.Builder builder) {}
    }

    public class I2 implements BlockHoundIntegration {
        public void applyTo(BlockHound.Builder builder) {}
    }

Now, let's say one want to install two more SPI integration plugin I3,I4; but ensuring the following:

  • I3 applyTo method must be called after I1, I2 (or I2, I1).
  • I4 applyTo method must be called after I3

then one could write this:

    public abstract class OrderedPlugin implements BlockHoundIntegration {
        final int id;

       public OrderedPlugin(int id) {this.id = id;}

        @Override
        public int compareTo(BlockHoundIntegration o) {
            if (o instanceof OrderedPlugin) {
                return Integer.compare(this.id, ((OrderedPlugin) o).id);
            }
            return 1; // always be placed after default plugins
        }
    }

    public class I3 extends OrderedPlugin {
        public I3() { super(1); }

        public void applyTo(BlockHound.Builder builder) {...}
    }

    public class I4 extends OrderedPlugin {
        public I4() { super(2); }

        public void applyTo(BlockHound.Builder builder) {...}
    }

The problem is that it does not work because CompareTo method is not transitive: when sorting, if I1.compareTo(OrderedPlugin) is called, then 0 is returned, hence we can't really sort the list like we want.

as a work around, you need to programatically install I3, I4, like this: this will ensure that plugins will be applied in desired order: I1, I2, I3, I4, but you then can't declare I3, I4 as SPI plugins.

    BlockHound.install(new I3(), new new I4());

now, @johnrengelman suggests a simple patch from #273: in BlockHoundIntegration, introduce a new getPriority method that is called by default by the compareTo method:

public interface BlockHoundIntegration extends Comparable<BlockHoundIntegration> {
    // ...
    default int getPriority() {
        return 0;
    }

    @Override
    default int compareTo(BlockHoundIntegration o) {
        return Integer.compare(getPriority(), o.getPriority());
    }

This resolves the issue, and plugins can simply refine the getPriority() method is they want to take control on the order, else , by default, plugins applyTo methods will be called in the order on which plugins are loaded (natural ordering).

One thing: in netty, the BlockHound integration is overriding the compareTo method , so, for completeness, a PR should be done on netty asking to not override the compareTo method.

Fixes #273

@pderop pderop added type/bug A general bug status/has-workaround This has a known workaround described labels Jan 24, 2023
@pderop pderop added this to the 1.0.7.RELEASE milestone Jan 24, 2023
@pderop pderop self-assigned this Jan 24, 2023
@pderop pderop requested a review from a team January 24, 2023 14:46
@pderop pderop changed the title SPI BlockhoundIntegration plugins ordering BlockhoundIntegration SPI plugins ordering Jan 24, 2023
Copy link
Member

@simonbasle simonbasle left a comment

Choose a reason for hiding this comment

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

LGTM

@pderop
Copy link
Contributor Author

pderop commented Jan 25, 2023

@simonbasle , thanks for the review !

@pderop pderop merged commit 0e7714e into reactor:master Jan 25, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status/has-workaround This has a known workaround described type/bug A general bug
Projects
None yet
2 participants