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

Bridge between Reactive-Streams and JDK 9 Flow API #296

Merged
merged 6 commits into from Sep 30, 2017

Conversation

@akarnokd
Copy link
Contributor

@akarnokd akarnokd commented Nov 21, 2015

This PR adds the code to bridge the Reactive-Streams API and the JDK 9 Flow APIs.

Unfortunately, I don't have any idea how to enable JDK 9 compilation or if it is even possible with gradle/travis at this point.

@ldaley
Copy link
Contributor

@ldaley ldaley commented Nov 22, 2015

Gradle has limited support for it. Compilation works, but executing tests with 9 does not. It's in progress, no ETA.

@viktorklang
Copy link
Contributor

@viktorklang viktorklang commented Nov 25, 2015

Thanks @akarnokd! Let's keep this here until we find a way to make Gradle not stand in our way :)

@akarnokd akarnokd force-pushed the akarnokd:FlowBridge branch from e718263 to 6a07583 Apr 14, 2016
@akarnokd
Copy link
Contributor Author

@akarnokd akarnokd commented Apr 14, 2016

Project Reactor uses a JSR166 backport for Flow interactions; I've added a dependency for it. Build for Java 8 passes but the backport won't work with Java 6.

@viktorklang
Copy link
Contributor

@viktorklang viktorklang commented Apr 18, 2016

@akarnokd thanks for the update! I guess Gradle and Java9 support is still not on the horizon?

@viktorklang
Copy link
Contributor

@viktorklang viktorklang commented Nov 21, 2016

@alkemist Do you happen to know if Gradle works with java 9 now?

@akarnokd
Copy link
Contributor Author

@akarnokd akarnokd commented Nov 21, 2016

@viktorklang Gradle 3+ should work, but that's 1/3 of the problem: another problem is that Travis doesn't support Java 9 builds yet afaik plus this project's setup requires a Java 6 compatible source in every subproject and currently it fails with class file has wrong version 52.0, should be 50.0.

@viktorklang
Copy link
Contributor

@viktorklang viktorklang commented Nov 21, 2016

@akarnokd If we cannot update the build file to support jdk9 we could most definitely create a subproject and have that properly configured. About TravisCI there's: travis-ci/travis-ci#5520 (comment)

@ktoso ktoso mentioned this pull request Jun 30, 2017
@akarnokd akarnokd force-pushed the akarnokd:FlowBridge branch 2 times, most recently from 5b8ac22 to fd50125 Jul 12, 2017
@akarnokd
Copy link
Contributor Author

@akarnokd akarnokd commented Jul 12, 2017

I've updated the PR to include conditional building of the Java 9 parts. Unfortunately, Travis-CI was not too cooperative; sorry about the commit spams. A few highlights:

  • Java 9 build requires the trusty CI environment
  • Java 9 requires Gradle 4+
  • trusty doesn't support openjdk6
  • Jdk 6 and 7 can't download Gradle 4+ through https due to cipher issues
  • couldn't figure out if there is a way to run openjdk6 on normal and the rest on trusty via the yml
  • Gradle 4+ doesn't support jdk 6
@akarnokd akarnokd force-pushed the akarnokd:FlowBridge branch from fd50125 to 53aba05 Jul 12, 2017
.travis.yml Outdated
install:
# Display Gradle, JVM and other versions
- ./gradlew -version

This comment has been minimized.

@ktoso

ktoso Jul 13, 2017
Contributor

Copied from my PR, could attribute as commit?
#383

This comment has been minimized.

@akarnokd

akarnokd Jul 13, 2017
Author Contributor

Okay.

@@ -3,7 +3,7 @@ subprojects {
apply plugin: "osgi"

group = "org.reactivestreams"
version = "1.0.1-RC2"
version = "1.0.0"

This comment has been minimized.

@ktoso

ktoso Jul 13, 2017
Contributor

why?

This comment has been minimized.

@ktoso

ktoso Jul 13, 2017
Contributor

For back releasing we'd do so separately if needed

This comment has been minimized.

@akarnokd

akarnokd Jul 13, 2017
Author Contributor

The original PR predated 1.0.1-RC2. I'll restore the version number.

This comment has been minimized.

@ktoso

ktoso Jul 13, 2017
Contributor

Ah I see, thanks :)

include ':reactive-streams'
include ':reactive-streams-tck'
include ':reactive-streams-examples'

if (jdkFlow) {
include ':reactive-streams-flow-bridge'
}

This comment has been minimized.

@ktoso

ktoso Jul 13, 2017
Contributor

ok, nice 👍

import java.util.concurrent.Flow;

/**
* Bridge between Reactive-Streams API and the Java 9 Flow API.

This comment has been minimized.

@ktoso

ktoso Jul 13, 2017
Contributor

javadoc link?

This comment has been minimized.

@akarnokd

akarnokd Jul 13, 2017
Author Contributor

Not sure what links do you expect here.

This comment has been minimized.

@ktoso

ktoso Sep 22, 2017
Contributor

I meant {@link java.util.concurrent.Flow}


/**
* Converts a Flow Publisher into a Reactive Publisher.
* @param <T> the value type

This comment has been minimized.

@ktoso

ktoso Jul 13, 2017
Contributor

element to use same wording as elsewhere

This comment has been minimized.

@akarnokd

akarnokd Jul 13, 2017
Author Contributor

Okay.


@Override
public Publisher<Integer> createFailedPublisher() {
return null;

This comment has been minimized.

@ktoso

ktoso Jul 13, 2017
Contributor

should be possible to provide one

This comment has been minimized.

@akarnokd

akarnokd Jul 13, 2017
Author Contributor

Okay.

return 100;
}

}

This comment has been minimized.

@ktoso

ktoso Jul 13, 2017
Contributor

Thought: Should we add test classes that work with the JDK9 interfaces directly right away when JDK9 is out?

This comment has been minimized.

@akarnokd

akarnokd Jul 13, 2017
Author Contributor

Maybe, but perhaps in its own module.

This comment has been minimized.

@ktoso

ktoso Jul 13, 2017
Contributor

Yeah, could be cleaner that way hm... Ticketified #388

@ktoso
Copy link
Contributor

@ktoso ktoso commented Jul 13, 2017

Naming bikeshed, but important one I think: I would not want to refer to Reactive Streams as "the Reactive one" or "toReactive", it sounds misleading. Even if the alternative sounds boring and long I think it's more correct and less misleading ("wha!? The Java one is not 'Reactive'?" - of course we know that's not why the name / not what it means, but it reads like that and adapters may be used by people completely new to the concept)


matrix:
include:
- jdk: openjdk7

This comment has been minimized.

@ktoso

ktoso Jul 13, 2017
Contributor

I think we may be OK not running against 6, but let's see what others say hm

This comment has been minimized.

@akarnokd

akarnokd Jul 13, 2017
Author Contributor

Not much option here I think, JDK 6 is less and less supported by Gradle/Travis. As long as the compile target is 6, we should be okay.

This comment has been minimized.

@ktoso

ktoso Jul 13, 2017
Contributor

👍

.travis.yml Outdated
- jdk: oraclejdk9 # JDK 9-ea+174 provided by Travis will be replaced by 9+177 in "before_install"
before_install:
- cd ~
- wget http://download.java.net/java/jdk9/archive/177/binaries/jdk-9+177_linux-x64_bin.tar.gz

This comment has been minimized.

@akarnokd

akarnokd Jul 25, 2017
Author Contributor

FYI, looks like this has been deleted and needs to be updated whenever there is a newer JDK build.

This comment has been minimized.

@viktorklang

viktorklang Jul 25, 2017
Contributor

If HTTP only supported redirects! ;)

@viktorklang
Copy link
Contributor

@viktorklang viktorklang commented Sep 22, 2017

@reactive-streams/contributors Time to revive this PR? Looks like Travis should support oraclejdk9 OOTB now. @akarnokd, care to update the PR?

.travis.yml Outdated
# From https://github.com/reactive-streams/reactive-streams-jvm/pull/383
- jdk: oraclejdk9 # JDK 9-ea+174 provided by Travis will be replaced by 9+177 in "before_install"
before_install:
- cd ~

This comment has been minimized.

@akarnokd

akarnokd Sep 22, 2017
Author Contributor

This is no longer needed on Travis, jdk: oraclejdk9 switches to 9b181 == GA

@akarnokd
Copy link
Contributor Author

@akarnokd akarnokd commented Sep 22, 2017

Upgrade done.

.travis.yml Outdated
- export JAVA_HOME=~/jdk-9
- PATH=$JAVA_HOME/bin:$PATH
- cd -
- jdk: oraclejdk9

This comment has been minimized.

@ktoso

ktoso Sep 22, 2017
Contributor

Great! :)

@ktoso
Copy link
Contributor

@ktoso ktoso commented Sep 22, 2017

Looking good, I'll review later tonight or tomorrow in more depth.
Late here and gotta do some family time

@viktorklang
Copy link
Contributor

@viktorklang viktorklang commented Sep 22, 2017

Copy link
Contributor

@ktoso ktoso left a comment

Just skimmed before heading out, but I think there's naming and docs to be discussed here. Will continue review soon, sorry for delay, late here.

}

/**
* Converts a Flow Publisher into a Reactive Publisher.

This comment has been minimized.

@ktoso

ktoso Sep 22, 2017
Contributor

Please "Reactive Streams Publisher/Subscriber" everywhere, let's be specific; the putting "Reactive" around imprecisely leads to people misunderstanding it and saying "oh yeah but the Flow one is not Reactive!" ;)

Copy link
Contributor

@ktoso ktoso left a comment

Just skimmed before heading out, but I think there's naming and docs to be discussed here. Will continue review soon, sorry for delay, late here.

Heh, seems if one's not blessed a request-changes does not mark the PR as such... good to know, sorry for double posting - thought I clicked the wrong thing.

Copy link
Contributor

@ktoso ktoso left a comment

Call me boring with naming, but spotted one more place; Impls look good, so LGTM, with the naming of one class to be optionally addressed.

Thanks for your work on this one @akarnokd :) 👍

@@ -100,7 +100,7 @@ private ReactiveFlowBridge() {
org.reactivestreams.Processor<? super T, ? extends U> reactiveStreamsProcessor
) {
if (reactiveStreamsProcessor == null) {
throw new NullPointerException("reactiveProcessor");
throw new NullPointerException("reactiveStreamsProcessor");

This comment has been minimized.

@ktoso

ktoso Sep 25, 2017
Contributor

👍

*/
@SuppressWarnings("unchecked")
public static <T> org.reactivestreams.Publisher<T> toReactive(
public static <T> org.reactivestreams.Publisher<T> toReactiveStreams(

This comment has been minimized.

@ktoso

ktoso Sep 25, 2017
Contributor

Thanks for the change! In docs this sounds much better, and in code it gets a bit long, but very precise which is good 👍

People won't usually call these "all the time" but just in a few places, so having a longer name should be fine.

/**
* Bridge between Reactive Streams API and the Java 9 {@link java.util.concurrent.Flow} API.
*/
public final class ReactiveFlowBridge {

This comment has been minimized.

@ktoso

ktoso Sep 25, 2017
Contributor

Same as commented previously, ReactiveStreamsFlowBridge. As this is directly visible user API

This comment has been minimized.

@akarnokd

akarnokd Sep 25, 2017
Author Contributor

Sure.

* @param <T> the input type
* @param <U> the output type
*/
static final class ReactiveToFlowProcessor<T, U>

This comment has been minimized.

@ktoso

ktoso Sep 25, 2017
Contributor

Those can remain R...Flow rather than RS I guess, since they're internal and people should not be seeing them directly

This comment has been minimized.

@akarnokd

akarnokd Sep 25, 2017
Author Contributor

Unless we consider stacktraces.

import java.util.concurrent.Flow;
import java.util.concurrent.TimeUnit;

class TestConsumer<T> implements Flow.Subscriber<T>, Subscriber<T> {

This comment has been minimized.

@ktoso

ktoso Sep 25, 2017
Contributor

TestEitherSubscriber?

This comment has been minimized.

@akarnokd

akarnokd Sep 25, 2017
Author Contributor

Okay.

@ktoso
ktoso approved these changes Sep 25, 2017
@akarnokd
Copy link
Contributor Author

@akarnokd akarnokd commented Sep 25, 2017

Updated based on @ktoso's feedback.

}
}

if (name in ["reactive-streams", "reactive-streams-tck", "reactive-streams-examples"]) {
if (name in ["reactive-streams", "reactive-streams-tck", "reactive-streams-examples", "reactive-streams-flow-bridge"]) {

This comment has been minimized.

@viktorklang

viktorklang Sep 25, 2017
Contributor

Not to start a bikeshedding exercise, but are we on board with the naming of this artifact?

Options are things like "reactive-streams-flow-compat", "reactive-streams-jdk9-compat", "reactive-streams-jdk9-interop" and the like.

This comment has been minimized.

@ktoso

ktoso Sep 29, 2017
Contributor

Heh yeah did not want to open that bikeshed as I've bikeshedded quite a lot here already.
Only precedent for such a library would be the https://github.com/scala/scala-java8-compat library I think, and the inclusion of the number there perhaps is sub optimal. So out of the ones you proposed I like reactive-streams-flow-compat I guess, or reactive-streams-java-flow-compat.

It's not super important to me though I guess, so would be interesting to hear or do a quick vote I guess?

This comment has been minimized.

@viktorklang

viktorklang Sep 29, 2017
Contributor

How about reactive-streams-jsr266-interop?

@ktoso
Copy link
Contributor

@ktoso ktoso commented Sep 29, 2017

Once this is merged I'll provide the TCK that can work on the flow interfaces directly btw, so eagerly awaiting this to be merged :)

@viktorklang
Copy link
Contributor

@viktorklang viktorklang commented Sep 30, 2017

@reactive-streams/contributors Given that this has had ample time for reviews, I'm going to merge this. Shall we back-release a version for this for 1.0.0 and 1.0.1 respectively?

@viktorklang viktorklang merged commit 306ae92 into reactive-streams:master Sep 30, 2017
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@akarnokd
Copy link
Contributor Author

@akarnokd akarnokd commented Sep 30, 2017

I guess renaming the logical module name can happen afterwards at any time. I'm fine with the merging otherwise. I'd target 1.0.1 only.

@akarnokd akarnokd deleted the akarnokd:FlowBridge branch Sep 30, 2017
akarnokd added a commit to akarnokd/reactive-streams that referenced this pull request Nov 3, 2017
* Repairs formatting issue of tables in spec README

* Modifies rules 1.09 and 2.13 to mandate `java.lang.NullPointerException` be thrown.

Updates the TCK, Spec and example implementations.

* Fixes reactive-streams#211 by clarifying

* Fixes reactive-streams#210 by removing 1.12 and repurposing its TCK checks for 1.09

* Clarifies the signalling sequence in the spec and
 adds TCK verification to ensure signal ordering is proper,
 also amends the examples to reflect the spec change.

* Publish 1.0.0.RC2
fix reactive-streams#215

* Fixes reactive-streams#217 by including the examples project in the publish task

* =tck minor test name fixup, it is a required test

* fix reactive-streams#212 issue on spec 213 testing wrt Processor

*  RC3 release /w reactive-streams#222 fix

* remove rule 1:12 (produce same elements to all Subscribers)

This rule is in conflict with 1:11 which allows a Publisher to treat
multiple Subscribers as either as unicast or multicast recipients. The
verification of proper multicast behavior (which 1:12 specified) has
been retained, the test methods renamed accordingly.

* fix three left-over references to deleted rule 1:12

* Fixed wrong footnote reference in README.md

* Addresses a couple of typos in the examples for AsyncSubscriber and SyncSubscriber

* !TCK clarify what error publisher is
+ add better readme on what this method is
+ add better javadoc on this method
- removes reference to old style spec annotation from readme
+ proposing to change method name to "createFailed..." as it is the
  wording used in the spec and reactive manifesto (footnote 1.1)
+ more info in tck/README that it is not legal to signal on* before sub
Resolves reactive-streams#237, reactive-streams#235

* +tck reactive-streams#236 example subscriber whitebox tested, and whitebox fixed

* add space to javadoc

* +TCK verifyNoAsyncErrors now by default waits, fixes spec111
Resolves reactive-streams#239

* =tck general tck/readme.md cleanup so it matches current code / spec
Resolves reactive-streams#99
Depends on reactive-streams#241

* Addresses PR review comments for reactive-streams#246

* Update CopyrightWaivers.txt

* +tck explains createElement in more useful terms

resolves reactive-streams#231

* +tck reactive-streams#232 explain which tests are mendatory to be "compliant"

* Update SubscriberWhiteboxVerification.java

Fixes Javadoc generation on Java8+ by having to manually qualify nested classes.

* Fixes reactive-streams#233 by implementing support for triggered demand in in the SubscriberBlackboxVerification

* Travis PR validation using both JDK 6 and 8

By validating on both JDKs we know the project even builds on 8,
while not using features (classes) from JDK8 - so it's still usable for JDK6 projects.

Resolves reactive-streams#254

* Small touchups to the TCK README.md

* Release 1.0.0.RC4

* Cancel the subscription after receiving all of the pertinent emissions (reactive-streams#259).

* Test that 'required_spec317_mustNotSignalOnErrorWhenPendingAboveLongMaxValue' completes in a timely manner for fully synchronous publishers (reactive-streams#259).

* =tck untested spec308 rule method name adjusted

* -tck rm undocumented and unused publisherReferenceGCTimeoutMillis method

* update version to 1.0.0.RC5

* Updating documentation to reflect the current version: RC5

* update ref to 1.0.0.final

* change 1.0.0.final to 1.0.0 and make sure OSGI manifest has the bundle version

*  OSGI fix

*  OSGI fix...

* Disambiguate "processing elements"

The document generally refers to "elements" as objects traversing a stream. I initially considered simply editing "processing elements" to read "processing components", but there's a section devoted to the definition of this, so better to link them.

* Added per request of @viktorklang in reactive-streams#269

* add CC0 label to README

* =tck reactive-streams#279 improve completion latch error message

* Rename SyncSybscriber.foreach to whenNext

* Update README.md

Spelling of the company name is Red Hat, not RedHat.

* I hereby represent [...] public domain [...] entirety of my contributions.

Requested by @viktorklang.

* Log test output events to the console

* Remove "preview" qualifier from README.

* Unbreaks TravisCI OpenJDK6 hostname too long crash

* Second attempt at unbreaking the Travis build

* Third attempt at fixing the Travis builds

* +tck reactive-streams#308 allow configuring "no events during N time" separately

* Update to Gradle 2.12

* Reintegrate dangling footnote in Publisher section.

- integrate the footnote in rule 1.9
- sign the Copyright Statement

* Asynchronous vs Synchronous Processing: reword "push-based stream"

* =tck fixes minor misalignment between code and comment, found via .NET port

Semantics remain exacly the same, the error we're testing here is about
signaling one more element if request comes in again (which we'll do
anyway, regardless of status of this flag)

* adjust Subscription.cancel javadoc because cancel command does not have to be called asynchronously

* Updating Typesafe to Lightbend

* Fix a typo in org.reactivestreams.example.unicast.AsyncSubscriber

* Add @seratch to CopyrightWaivers.txt

* Fixes reactive-streams#333 by adding license headers to /examples/*

* Adds a Glossary, Intent-sections and harmonizes verbiage

* Clarifying that object equality is a.equals(b) in Intent for 2.12

* add license header to API directory

* add license header to TCK

* Fix missing cancel() from in tests that don't consume the entire source

* Run with default TestEnvironment settings.

* Update CopyrightWaivers.txt

* =build reactive-streams#349 equal osgi manifest version as real version

To have a tangible PR to talk about.
Probably enough to resolve reactive-streams#349

Would be followed up with change to 1.0.1 eventually.

* Add Javadoc explanation to the TCK test methods about what they do

* Don't import org.reactivestreams.tck.TestEnvironment

* Fix missing Javadoc tags

* TCK: Request -1 in 309 instead of a random non-positive number

* Remove the Random instance as well.

* Keep the randomness.

* Fixing typos in README.md

* Minor rewording of 2.6 to make it easier to understand. (reactive-streams#342)

* Minor rewording of 2.6 to make it easier to understand.

* Fix spelling errors and clarify a couple of sentences

* extra coordination

* Remove vague statements, be more specific in others

* Update javadoc based on ktoso's feedback

* Use the wording eagery for error publisher test 104

* Address feedback, add links to the rules in the javadoc

* SubscriberBlackboxVerificationRules explained

* Non-BC for TCK: Corrects a typo in test method from *Compuatation to *Computation

* Adding a glossary item for external synchronization

* Repointing links to sources in README to current main release

* =tck reactive-streams#362 signal onComplete in 201 blackbox verification

* +tck reactive-streams#362 complete subscriber under test once done in 205

* +tck reactive-streams#362 wait for request signal in 209, and new additional tests

* =tck check isCancelled in 205 blackbox; sample the state sometimes

* =tck reactive-streams#362 blackbox 209 must issue onSubscribe before any other signal

* Clarifies the meaning of "stochastic" for skipStochasticTests()

* add additional test for optional_spec111.

* now test verifies https://github.com/reactive-streams/reactive-streams-jvm#1.11 and
https://github.com/reactive-streams/reactive-streams-jvm#1.5 for publishers, if they support multiple subscribers.

* add delegate to IdentityProcessorVerification.

* add tests for optional_spec111_registeredSubscribersMustReceiveOnNextOrOnCompleteSignals.

* additional happy and the failure cases.
* clear typos and change comments.
* add new PublisherVerification for multi-subscribers tests.

* removed onSubscribe constructor call.

renamed Demand -> CancelableSubscription.

* Change subscription remove logic.

* add myself to CopyrightWaivers.

* fix tests by using proxied subscriber,
thanks Viktor for helping push this fix

* Be consistent in reference style

We use the `#.##` style in referring to rules everywhere, this one ref was using a different style - fixed that.

* Switching to consistent use of apostrophe in spec

* More apostrophe fixes

* add patriknw to CopyrightWaivers

* Version 1.0.1

* =spec reactive-streams#384 amend spec to allow not mentioning rule number in exception message

* Update README.md

* =tck reactive-streams#384 dont check for cause message when checking 3.9

* Updating versions to 1.0.1-RC2 and clarifying changes in RELEASE-NOTES.md

* Fix links to "Terminal state" (reactive-streams#389)

* Fix links to "Terminal state"

* add angelsanz to CopyrightWaivers.txt

* Preparing 1.0.1 (reactive-streams#390)

* Bridge between Reactive-Streams and JDK 9 Flow API (reactive-streams#296)

* Bridge between Reactive-Streams and JDK 9 Flow API

* Apply changes based on ktoso's feedback

* Use oraclejdk9, resolve build.gradle conflict

* Change txt/code to use "Reactive Streams" as designator

* NPE to use the updated parameter name.

* Rename bridge class, tester class (+javadoc)

* Java 9 Flow bridge: add Subscriber converters (reactive-streams#399)

* Java 9 Flow bridge: add Subscriber converters

* Fix return type javadoc

* Example synchronous range Publisher (reactive-streams#395)

* Example synchronous range Publisher

* Udpated with rule numbers in comments

* Mentioning rule 3.9 again in emit()

* Move classes to the unicast package.

* [WIP] TCK for j.u.c.Flow types "directly" (reactive-streams#398)

* Add JDK9 TCK, using adapters

* Fixing wrapping and unwrapping of the wrappers themselves.

* Renames the converters to "toX" for RS and "toFlowX" for Flow.

Fixes so that the dist url for gradle is http iso https (TravisCI bug?)

Adds regression test for bridge converters.

* fix formatting

* cleanup
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants