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.

Sign up for GitHub

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

Jump to bottom

JDK9 Flow integration #1783

Merged
merged 8 commits into from
Mar 13, 2020
Merged

JDK9 Flow integration #1783

merged 8 commits into from
Mar 13, 2020

Conversation

dkhalanskyjb
Copy link
Collaborator

Solves #162, #1727

@fvasco
Copy link
Contributor

fvasco commented Feb 4, 2020

Even though the reactive-streams-flow-adapters simplifies this module a lot, it is an important dependency.
This PR looks like a shortcut library to the FlowAdapters.toPublisher method, not so much in my humble opinion.

@dkhalanskyjb dkhalanskyjb force-pushed the kotlinx-coroutines-jdk9 branch 2 times, most recently from 3f15dba to 5d0ba5e Compare February 7, 2020 08:13
@elizarov elizarov force-pushed the jdk-11 branch 3 times, most recently from 8c1addf to 339a367 Compare February 14, 2020 08:12
qwwdfsad
qwwdfsad approved these changes Feb 21, 2020
View reviewed changes
Copy link
Collaborator

@qwwdfsad qwwdfsad left a comment

Choose a reason for hiding this comment

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

Great job

@dkhalanskyjb
Implement Java9 reactive streams integration
3fff4a7 
This was accomplished by copying `kotlinx-coroutines-reactive`.
Judging by the fact that
https://github.com/reactive-streams/reactive-streams-jvm/blob/master/api/src/main/java9/org/reactivestreams/FlowAdapters.java
defines converters between Java9 Flow and Reactive Streams as thin
wrappers that simply redirect methods, this should be enough.

Divergences from `kotlinx-coroutines-reactive` are as follows:
* The target bytecode is set to JDK9.
* JDK11 is required to build the module.
* Automated change of all occurrences `org.reactivestreams` to
  `java.util.concurrent.Flow`.
* Removal of `RangePublisherTest`, `RangePublisherBufferdTest`,
  and `UnboundedIntegerIncrementPublisherTest`. They all are heavily
  based on the examples provided for the Reactive Streams, and
  to use them for testing this integration would only be possible with
  heavy use of `FlowAdapters`, which seems redundant as the integration
  with the examples themselves is already tested in
  `kotlinx-coroutines-reactive`, and the correctness of the wrappers is
  probably a given.
* Use of `FlowAdapters` where needed to make everything valid code.
@dkhalanskyjb
Remove deprecated methods from the Java Flow integration module
f5b488a 
The `kotlinx-coroutines-jdk9` module, being a copy of
`kotlinx-coroutines-reactive`, has its share of legacy APIs
defined. As this is a new module, there is no reason to
preserve them.

The changes include:
* `Migration.kt`, being a file completely dedicated to warnings
  about old API usage, has been removed.
* `IntegrationTest.kt` changed slightly so that it no longer uses
  the subscription channel API, which is deprecated.
* `Channel.kt` is now just an implementation detail. It does not
  expose any public methods.
* In particular, `Publisher<T>.collect` has been moved to
  `ReactiveFlow.kt` and is no longer inline, as it would expose
  `openSubscription`, which is deprecated.
* `PublisherSubscriptionSelectTest`, which tests use of `select`
  with the subscription channel API, is not included anymore.
* `Convert.kt` also has been removed altogether, having no
  non-deprecated methods.
@dkhalanskyjb
Remove context injection from JDK9 Flow integration
dafe32c
@dkhalanskyjb
Simplify JDK9 Flow integration by calling ReactiveStreams integration
abf2090
@dkhalanskyjb
Remove TCK tests from JDK9 Flow integration
97f9030 
Since the JDK9 Flow integration is now implemented as thin
wrappers around the Reactive Streams integration, there is no need
for such thorough testing, and additional tests would only cause
the overhead of needing to fix two copies at once when changing
the Reactive Streams integration.
@dkhalanskyjb
Remove mentions of ReactorContext from JDK 9 Flow integration doc
dd4a9dd
@dkhalanskyjb
Refactoring: remove assertNSE and assertIAE
2a383c8
@dkhalanskyjb dkhalanskyjb changed the base branch from jdk-11 to develop March 5, 2020 08:34
elizarov
elizarov suggested changes Mar 10, 2020
View reviewed changes
Copy link
Contributor

@elizarov elizarov left a comment

Choose a reason for hiding this comment

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

This code would be easier to

reactive/kotlinx-coroutines-jdk9/src/Await.kt Outdated Show resolved Hide resolved
reactive/kotlinx-coroutines-jdk9/src/Publish.kt Outdated Show resolved Hide resolved
@qwwdfsad qwwdfsad force-pushed the develop branch 2 times, most recently from 4a49830 to aff8202 Compare March 10, 2020 17:27
@dkhalanskyjb
Copy link
Collaborator Author

Since this subproject outright requires to be built under JDK 9+, calling gradle build here with JDK 1.8 will not work properly. It should probably be dealt with, either by dropping support for JDK 1.8 and changing the build instructions in the top-level README to require JDK 11, or by building this subproject only conditionally. @elizarov , what would be the right thing to do?

@elizarov
Copy link
Contributor

Since this subproject outright requires to be built under JDK 9+, calling gradle build here with JDK 1.8 will not work properly. It should probably be dealt with, either by dropping support for JDK 1.8 and changing the build instructions in the top-level README to require JDK 11, or by building this subproject only conditionally. @elizarov , what would be the right thing to do?

It is already mentioned in top-level readme:

  • JDK >= 11 referred to by the JAVA_HOME environment variable.

@elizarov elizarov merged commit a25bf36 into develop Mar 13, 2020
@qwwdfsad qwwdfsad deleted the kotlinx-coroutines-jdk9 branch March 16, 2020 14:55
@dkhalanskyjb dkhalanskyjb mentioned this pull request Mar 23, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@fvasco fvasco fvasco left review comments

@LouisCAD LouisCAD LouisCAD left review comments

@qwwdfsad qwwdfsad qwwdfsad approved these changes

@elizarov elizarov elizarov approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@dkhalanskyjb @fvasco @elizarov @qwwdfsad @LouisCAD