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

Expose async tls_handler #448

Merged
merged 8 commits into from
Sep 13, 2022
Merged

Conversation

mbacarella
Copy link
Contributor

I proprose this PR to expose the async tls_handler to users of the library. This is needed if you want to do your own Tcp service management, such as if you're plugging async tls support into other frameworks.

@hannesm
Copy link
Member

hannesm commented Aug 5, 2022

that's a pretty huge diff for the description. any chance you could remove any code formatting changes from this PR?

also, maybe @torinnd could take a look at the changes?

@torinnd
Copy link
Contributor

torinnd commented Aug 5, 2022

I'd probably recommend collapsing the new test_server_low_level example into the test_server. They don't look different enough to me to justify persisting both.

I also think I'd refactor the MLI as follows:

type 'a io_handler = Reader.t -> Writer.t -> 'a Deferred.t
type 'a tls_handler = Session.t -> 'a io_handler

val upgrade_server_handler
  : config:Tls.Config.server
  -> 'a tls_handler
  -> 'a io_handler

Notably this excludes any mention of the socket address from the type signature as that isn't relevant to the TLS upgrade being performed here. Callers would probably do something like upgrade_server_handler ~config (my_cool_handler socket) to satisfy the Async.Tcp.Server.create interface.

@mbacarella
Copy link
Contributor Author

Thanks for the review. Collapsing the test servers into one and the .mli refactor done.

I believe all of the extraneous code formatting has been removed (mostly in the dune file).

async/tls_async.mli Outdated Show resolved Hide resolved
@hannesm hannesm merged commit 9a26701 into mirleft:main Sep 13, 2022
@hannesm
Copy link
Member

hannesm commented Sep 13, 2022

Thanks for your contribution. I squashed and merged, this will be part of the next release.

hannesm added a commit to hannesm/opam-repository that referenced this pull request Sep 27, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants