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

#3649 - Add ZStream.fromTcpSocketServer #3677

Merged
merged 3 commits into from Jun 6, 2020

Conversation

regis-leray
Copy link
Member

@regis-leray regis-leray commented May 21, 2020

closes #3649

@regis-leray regis-leray requested a review from iravid as a code owner May 21, 2020 14:07
@regis-leray regis-leray force-pushed the 3649-ZStream.fromTcpSocketServer branch 2 times, most recently from 8a969f1 to ead4739 Compare May 21, 2020 17:01
@iravid iravid added the stream ZIO Stream label May 21, 2020
@iravid
Copy link
Member

iravid commented May 21, 2020

@regis-leray Nice work. This is looking good.

Can we switch to using AsynchronousServerSocketChannel here? See https://stackoverflow.com/questions/22177722/java-nio-non-blocking-channels-vs-asynchronouschannels for reasoning.

@regis-leray regis-leray force-pushed the 3649-ZStream.fromTcpSocketServer branch from ead4739 to f107be1 Compare May 21, 2020 21:58
@regis-leray
Copy link
Member Author

@iravid thanks for the the tips asynchronous, i tested locally it works. missing test and it will be ready to merge

Copy link
Member

@iravid iravid left a comment

Choose a reason for hiding this comment

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

Thank you, this looks great.

Left a few more requests. Tests can go in ZStreamPlatformSpecificSpec.scala.

streams/jvm/src/main/scala/zio/stream/platform.scala Outdated Show resolved Hide resolved
streams/jvm/src/main/scala/zio/stream/platform.scala Outdated Show resolved Hide resolved
streams/jvm/src/main/scala/zio/stream/platform.scala Outdated Show resolved Hide resolved
@regis-leray regis-leray force-pushed the 3649-ZStream.fromTcpSocketServer branch from f107be1 to bdd2b7c Compare May 22, 2020 12:51
Copy link
Member

@iravid iravid left a comment

Choose a reason for hiding this comment

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

Left a few minor suggestions while this is still open.

streams/jvm/src/main/scala/zio/stream/platform.scala Outdated Show resolved Hide resolved
streams/jvm/src/main/scala/zio/stream/platform.scala Outdated Show resolved Hide resolved
@regis-leray regis-leray force-pushed the 3649-ZStream.fromTcpSocketServer branch from bdd2b7c to b390785 Compare May 23, 2020 01:28
def fromSocketServer(
port: Int,
host: Option[String] = None
): ZStream[Blocking, Throwable, UManaged[Connection]] =
Copy link
Member

Choose a reason for hiding this comment

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

What do you think about using ZManaged#scope to pull the manageds into the stream itself?
This leads to a far more ergonomic signature and is still resource safe as all connections will be closed when the stream is closed.

Something like this https://github.com/zio/zio-keeper/blob/master/keeper/src/main/scala/zio/keeper/transport/tcp.scala

Copy link
Member

Choose a reason for hiding this comment

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

Yes I think that'd be good. ZStream[Connection] is indeed nicer than ZStream[Managed[Connection]]. What do you think @regis-leray?

Copy link
Member Author

Choose a reason for hiding this comment

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

👍

@iravid
Copy link
Member

iravid commented May 25, 2020

We just need a test or two after this @regis-leray and this should be good to go.

@regis-leray regis-leray force-pushed the 3649-ZStream.fromTcpSocketServer branch 2 times, most recently from 3b0ac92 to ac218e8 Compare June 2, 2020 17:24
@regis-leray
Copy link
Member Author

@iravid sorry for the delay. I provided some test.

@regis-leray regis-leray force-pushed the 3649-ZStream.fromTcpSocketServer branch from ac218e8 to 849ebef Compare June 2, 2020 23:41
@regis-leray
Copy link
Member Author

@iravid i added some javadoc

@regis-leray regis-leray force-pushed the 3649-ZStream.fromTcpSocketServer branch from 9fcc4d8 to dd1bd2a Compare June 6, 2020 14:13
Copy link
Member

@iravid iravid left a comment

Choose a reason for hiding this comment

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

One last tweak then good to merge. Thanks @regis-leray

}
)
}
} >>= (ZStream.managed(_))
Copy link
Member

Choose a reason for hiding this comment

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

This is subtly wrong, because it means that as soon as you progress to the next connection, the previous connection is closed. Instead, let's use ZManaged.scope here:

for {
  server <- ZStream.managed(...)
  registerConnection <- ZStream.managed(ZManaged.scope)
  conn <- ZStream.repeatEffect {
                 IO.effectAsync(...).flatMap(managedConn => registerConnection(managedConn).map(_._2))
                }
} yield conn

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks for the tip ! i was not aware of this ZManaged.scope

@regis-leray regis-leray force-pushed the 3649-ZStream.fromTcpSocketServer branch from dd1bd2a to d9a0956 Compare June 6, 2020 14:35
@iravid iravid merged commit 803acb6 into zio:master Jun 6, 2020
@iravid
Copy link
Member

iravid commented Jun 6, 2020

Great work @regis-leray, thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
stream ZIO Stream
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add ZStream.fromTcpSocketServer
3 participants