eclipse / microprofile-reactive-streams-operators

Microprofile project
Apache License 2.0
80 stars 33 forks source link

Use interfaces instead of concrete classes #98

Closed jroper closed 6 years ago

jroper commented 6 years ago

This is a big change, so it's probably better to read over the philosophy behind the change, and check it out somewhere to review the code locally to see the exact approach.

What this does is replace all concrete classes in both the API and SPI with interfaces. It also provides a core implementation of the API as a separate project. The SPI has also been put in a separate project.

From a users perspective, none of the APIs have changed.

From an SPI implementors perspective, there are a few changes. The biggest change is that since all the stages are no longer concrete classes, you can't just use stage.getClass() to then lookup a stage from a map of stage classes to handlers for the stage, rather you need to recurse over the class tree to do it. Alternatively, iterating through all stages and doing an instanceof will work.

A ReactiveStreamsFactory interface that mirrors the ReactiveStreams static methods has been created, implementations of this get looked up using the JDK service loader.

Due to the split of the SPI from the API, the SPI has had to introduce its own CompletionSubscriber interface. It has the same name as the one in the API, which may be a bit confusing, but I'm not sure what else to call it.

I haven't at all attempted to address splitting the API into multiple interfaces for the purpose of grouping and discoverability, but that's unrelated to this change, and this change is already big enough. It will be valuable to discuss excatly how things get split up, and discussions about the two changes are better had separately. So that can be done in a different PR.

jroper commented 6 years ago

I've pushed a commit that has two changes - I probably should have done them in separate commits.

The first change renames spi.CompletionSubscriber to SubscriberWithCompletionStage. Having both CompletionSubscriber in the API and in the SPI makes things very awkward and confusing for implementations, so I've renamed it to SubscriberWithCompletionStage in the SPI, to differentiate between the two classes.

The second change removes Graph.hasInlet/hasOutlet and Stage.hasInlet/hasOutlet. These serve no functional purpose, for stages, the booleans they return can be inferred by the type of stage, and for graphs, the information can be inferred by inspecting the graph. They were used primarily for validation of the graph that the builder was building, but since the builder is in complete control of the building, and its implementation ensures that it never builds invalid graphs, this validation doesn't serve any useful purpose beyond detecting fairly trivial bugs.

Some implementations of the SPI may depend on these methods, but they can easily obtain the information by inspecting the stage/graph.

jroper commented 6 years ago

@cescoffier so are you happy for this to be merged? Have you seen how it impacts your impl? The move to interfaces for stages means simple hash map based lookups of stage classes won't work. My implementation of this so far is:

https://github.com/lightbend/microprofile-reactive-streams/blob/82954d295ff8715da08d9a908c3f3e6a5cbf257a/akka/src/main/java/com/lightbend/microprofile/reactive/streams/akka/AkkaEngine.java#L346-L372

I think in practice that shouldn't perform too badly, though to make it really perform well I'd want to, for a given stage impl class, cache the result of the lookup, so that next time it will just be a simple hash map lookup. My reason for not doing that yet is that it adds the potential for classloader leaks, so either have to use a hash map with weak keys, or somehow ensure the lifecycle of the stage impl classes is at least as wide as the lifecycle of the engine. Generally they will match, but it's not guaranteed, eg if a user implemented their own stage classes for whatever reason, then that would very likely be a classloader leak in dynamic classloading environments.

cescoffier commented 6 years ago

@jroper I'm mostly good with the PR. I would just add one comment. Can org.eclipse.microprofile.reactive.streams.CompletionSubscriber#of be made public? It would avoid having to store an implementation of the class in the implementation of the spec.

About the dispatching, I've already adapted my implementation. I would avoid caching because it can make things tricky in dynamic environments (OSGi for instance) where the stage implementation could be updated on the fly (not sure there is a use case but well... we never know). For my dispatching implementation, I'm using a "simple" list looking for a stage "assignable to" the given stage class. Yes, of course, it's not a O(1) access, but looking at the number of stages we have the cost is small. I may revisit that when we have hundreds of stages.

jroper commented 6 years ago

Ok I'll merge. CompletionSubscriber.of is public, all methods on interfaces, static or otherwise, are public.

If there's anything else you wanted to discuss, please raise some issues, everything's up for changing if necessary.

cescoffier commented 6 years ago

I will have another look Today. Do you think we can have an RC2 tomorrow?

jroper commented 6 years ago

Not likely, I've got another PR to demonstrate grouping the functionality that I'll submit soon. It's also got a number of fixes where we missed or didn't get the co/contra-variance of arguments right.