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

Upgrade a JS socket to TLS directly if possible #3341

Open
wants to merge 8 commits into
base: main
Choose a base branch
from
Open

Upgrade a JS socket to TLS directly if possible #3341

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
f3091ce
Upgrade a js socket to TLS directly if possible
mikeshepherd Nov 20, 2023
40d2113
Fix bin compat issue
mikeshepherd Nov 20, 2023
7d7d543
Fix bincompat issues by ignoring
mikeshepherd Nov 21, 2023
02e788a
Remove `Dispatcher` from `suspendReadableAndRead`
armanbilge Nov 25, 2023
7874843
Don't use `Dispatcher` for TLS init errors
armanbilge Nov 26, 2023
297fc7d
Optimize
armanbilge Nov 27, 2023
12e95aa
Merge remote-tracking branch 'upstream/main' into use-js-socket-direc…
mikeshepherd Nov 28, 2023
b49edb5
Merge remote-tracking branch 'armanbilge/fix/3338' into use-js-socket…
mikeshepherd Nov 28, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 3 additions & 5 deletions io/js/src/main/scala/fs2/io/net/tls/TLSContextPlatform.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -67,7 +67,7 @@ private[tls] trait TLSContextCompanionPlatform { self: TLSContext.type =>
if (clientMode) {
Resource.eval(F.deferred[Either[Throwable, Unit]]).flatMap { handshake =>
TLSSocket
.forAsync(
.forAsyncClient(
socket,
sock => {
val options = params.toTLSConnectOptions(parDispatcher)
Expand All @@ -89,8 +89,7 @@ private[tls] trait TLSContextCompanionPlatform { self: TLSContext.type =>
)
)
tlsSock
},
clientMode = clientMode
}
)
.evalTap(_ => handshake.get.rethrow)
}
Expand Down Expand Up @@ -129,8 +128,7 @@ private[tls] trait TLSContextCompanionPlatform { self: TLSContext.type =>
)
)
tlsSock
},
clientMode = clientMode
}
)
.evalTap(_ => verifyError.get.rethrow)
}
Expand Down
12 changes: 12 additions & 0 deletions io/js/src/main/scala/fs2/io/net/tls/TLSSocketPlatform.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,18 @@ private[tls] trait TLSSocketPlatform[F[_]]
private[tls] trait TLSSocketCompanionPlatform { self: TLSSocket.type =>

private[tls] def forAsync[F[_]](
Copy link
Member

Choose a reason for hiding this comment

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

Should we rename this forAsyncServer?

Copy link
Member

Choose a reason for hiding this comment

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

Or couldn't we just pass clientMode in forAsync so we don't need separate methods? I don't really care, these are just nitpicks :)

Copy link
Member

Choose a reason for hiding this comment

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

Oh I see, it was bincompat. But this is all private anyway. We can just add the exclusion.

Copy link
Author

Choose a reason for hiding this comment

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

Yeah, I didn't want to just add the exclusion unilaterally. Since it was fairly easy to avoid I did, but I've gone back to what it was before ignored the error

Copy link
Member

Choose a reason for hiding this comment

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

Since it was fairly easy to avoid I did

Yeah, this is good practice so thanks for doing that. But in this case I think its cleaner like this.

socket: Socket[F],
upgrade: fs2.io.Duplex => facade.tls.TLSSocket
)(implicit F: Async[F]): Resource[F, TLSSocket[F]] =
forAsyncInternal(socket, upgrade, clientMode = false)

private[tls] def forAsyncClient[F[_]](
socket: Socket[F],
upgrade: fs2.io.Duplex => facade.tls.TLSSocket
)(implicit F: Async[F]): Resource[F, TLSSocket[F]] =
forAsyncInternal(socket, upgrade, clientMode = true)

private[this] def forAsyncInternal[F[_]](
socket: Socket[F],
upgrade: fs2.io.Duplex => facade.tls.TLSSocket,
clientMode: Boolean
Expand Down