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

Upgrade to cats-effect 2.x, preliminary scala 2.13 support #63

Merged
merged 1 commit into from
Aug 29, 2019
Merged

Upgrade to cats-effect 2.x, preliminary scala 2.13 support #63

merged 1 commit into from
Aug 29, 2019

Conversation

Daenyth
Copy link
Contributor

@Daenyth Daenyth commented Jul 22, 2019

  • Upgrade to cats-effect 2.x, fs2 1.1.x
  • Add api to avoid dropping Future[Mat] values
  • Deprecate apis hardcoding IO
  • Fix race condition in akka-fs2 publisherStream between interruption
    and publish when using the fromFuture+ContextShift.shift behavior
  • Test changes to make wording match assertions better
  • Add scala-collection-compat for camel converter crossbuilding
  • Bump version for milestone release
  • Include sbt-tpolecat plugin to prevent lintable bugs

Ref #61
Close #62

@Daenyth

This comment has been minimized.

@krasserm

This comment has been minimized.

@Daenyth
Copy link
Contributor Author

Daenyth commented Jul 30, 2019

I figured out the test failures - it's a race condition in publisherStream between interruptWhen and publish.

I also backported the workaround for #54 - the test description didn't match what the test was actually doing. Now it does.

There's api changes that address #62

@@ -44,7 +47,7 @@ trait Converter {
* Converts an Akka Stream [[Graph]] of [[SinkShape]] to an FS2 [[Sink]]. The [[Graph]] is materialized when
* the [[Sink]]'s [[F]] in run. The materialized value can be obtained with the `onMaterialization` callback.
*/
def akkaSinkToFs2Sink[F[_], A, M](sink: Graph[SinkShape[A], M])(onMaterialization: M => Unit)(implicit materializer: Materializer, F: Concurrent[F]): Sink[F, A] = { s =>
def akkaSinkToFs2Pipe[F[_]: ContextShift, A, M](sink: Graph[SinkShape[A], M])(onMaterialization: M => Unit)(implicit materializer: Materializer, F: Concurrent[F]): Pipe[F, A, Unit] = { s =>
Copy link
Contributor Author

Choose a reason for hiding this comment

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

fs2.Sink is deprecated

* The stream returned by this will emit the Future's value one time at the end,
* then terminate.
*/
def akkaSinkToFs2PipeMat[F[_]: ConcurrentEffect: ContextShift, A, M](akkaSink: Graph[SinkShape[A], Future[M]])(
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Provides an out of the box workaround for #54 and makes the unit test a bit more honest

.toMat(AkkaSink.head)(Keep.right)
.mapMaterializedValue(ffd => IO.fromFuture(IO.pure(ffd)).flatMap(fd => IO.fromFuture(IO.pure(fd))).unsafeToFuture())
.mapMaterializedValue(ffd => ffd.flatten)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Avoids a whole bunch of thread pool hops

val sink2 = AkkaSink.foreach[(SourceQueueWithComplete[B], SinkQueueWithCancel[A])](ps => F.toIO(transformerStream(ps._2, ps._1, pipe).compile.drain).unsafeToFuture())
val sink2 = AkkaSink.foreach[(SourceQueueWithComplete[B], SinkQueueWithCancel[A])] { ps =>
// Fire and forget Future so it runs in the background
F.toIO(transformerStream(ps._2, ps._1, pipe).compile.drain).unsafeToFuture()
Copy link
Contributor Author

Choose a reason for hiding this comment

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

It might be good to hook in cancellation support...

@@ -71,15 +71,15 @@ class ConverterSpec extends TestKit(ActorSystem("test")) with WordSpecLike with

expectError(stream.compile.drain.unsafeRunSync())
}
"propagate cancellation from stream to source (on stream completion)" in {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

wording change - cancellation is a specific thing in cats-effect, and this test doesn't cover it

- Upgrade to cats-effect 2.x, fs2 1.1.x
- Add api to avoid dropping Future[Mat] values
- Deprecate apis hardcoding `IO`
- Fix race condition in akka-fs2 `publisherStream` between interruption
  and publish when using the fromFuture+ContextShift.shift behavior
- Test changes to make wording match assertions better
- Add scala-collection-compat for camel converter crossbuilding
- Bump version for milestone release
- Include sbt-tpolecat plugin to prevent lintable bugs
@Daenyth Daenyth marked this pull request as ready for review August 2, 2019 17:39
@Daenyth Daenyth merged commit 0a01ff7 into krasserm:master Aug 29, 2019
@Daenyth Daenyth deleted the scala2.13 branch August 29, 2019 15:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Conversion from fs2.Stream to Akka Source Requires ContextShift[IO]
2 participants