-
Notifications
You must be signed in to change notification settings - Fork 607
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
Cleanup fs2-io networking support #2209
Cleanup fs2-io networking support #2209
Conversation
Assuming the library is decently documented, and it passed your bar for stability, I'm all for not having to mess around with weird java types. I think the introduction of scodec-bits, for example, payed nice dividends, and I think the same applies here :) |
I wrote it, though that might work against me on the "decently documented" front. :) Though I think this one is documented pretty well. |
…tcp socket groups
@@ -217,12 +214,11 @@ object TLSContext { | |||
.eval( | |||
engine( | |||
new TLSEngine.Binding[F] { | |||
def write(data: Chunk[Byte], timeout: Option[FiniteDuration]): F[Unit] = | |||
def write(data: Chunk[Byte]): F[Unit] = |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why did timeout get removed here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nevermind i see your comment now.
This PR cleans up a bunch of things in the fs2-io networking support. Some of these are API annoyances and others are performance enhancements. Goal is to be competitive with fs2-netty when using nio backend, though not epoll or io_uring backends.
Use ip4s
This is something that's bothered me for a very long time. Before this PR, we used
java.net.{InetAddress, InetSocketAddress}
in these APIs. These are pretty dangerous, as simple looking expressions end up with side effects and blocking. For example:new InetSocketAddress("google.com", 443)
does a DNS lookup and since it's not wrapped byF.blocking
, it blocks a thread on compute pool.In this PR, we use the types from https://github.com/comcast/ip4s, which are all immutable and safe. DNS resolution is handled via
.resolve[F]
onHost
.Since APIs change here, I'd appreciate comments on the approach even if you don't have the time or desire to do a whole review. :)
Manage SocketOptions Better
The various socket constructors took a bunch of standard socket options as params and defaulted them to adhoc values (e.g.
.open(receiveBufferSize = 256 * 1024)
). This PR gets rid of all of those defaulted parameters and instead takes a singleList[SocketOption]
whereSocketOption
is defined in fs2 and contains a constructor for each standard socket option as well as the ability to construct custom socket options. We'll get better defaults this way, as the JDK / OS will pick the unspecified values.Package Layout and Renames
This PR introduces a new
fs2.io.net
package which contains both TCP and UDP support. Thefs2.io.tls
package is nowfs2.io.net.tls
.The UDP types were renamed:
Packet
is nowDatagram
,Socket
is nowDatagramSocket
, andSocketGroup
is nowDatagramSocketGroup
.Remove Socket Timeouts
This PR also removes the optional timeout parameter from socket read/writes in favor of using effect timeouts. E.g.,
socket.read.timeout(1.minute)
. These were mostly holdovers from when effect types didn't support timeouts.Make SocketGroups Optional
Like Java NIO, we now have an internal set of global socket groups. We retain the ability to allocate explicit socket groups but 99.9% of applications should be able to use the global socket groups.
Simplify TCP Server Signatures
The
server
andserverResource
methods onSocketGroup
were unnecessarily complex. They've been simplified to take advantage of FS2 resource scoping rules. Theserver
operation now returns aStream[F, Socket[F]]
instead of aStream[F, Resource[F, Socket]]
. A similar simplification was made toserverResource
.Remove Socket#close
The
Socket#close
method was removed, as socket lifecycle is managed by theResource
it is created in.