Norelease+flush #106

craff · 2023-04-09T01:01:53Z

Two commits: exception in flush + functions that do not release the global loc in a separate module.

…d in test

craff · 2023-04-09T01:13:21Z

There is a compilation problem in a test related to Thread.exit () versus raise Thread.Exit.

we have to choose between keeping a deprecated warning in Tests (which gives an error with warn as error) or abandoning older ocaml version.

Tell me what you prefer...

craff · 2023-04-09T01:38:01Z

I fixed the problem in Test/util.ml with local [@ warning -3]

craff · 2023-04-09T02:08:12Z

Here are some tests with nginx+ssl (as reference) and simple_http+ssl on a 50MB file

number of connection	nginx-ssl	simple-ssl without PR	simple-ssl with PR
5	50.30	36.56	59.33
10	54.95	32.43	60.96
15	54.63	33.81	61.60
20	53.06	33.08	61.57

This PR brings almost a factor 2 and allows to be better that nginx!

craff · 2023-04-13T22:18:59Z

No news ???

anmonteiro · 2023-04-13T22:50:57Z

No news ???

This project is maintained by (unpaid) volunteers. Please allow some time before we can review your changes (for free! Isn’t that beautiful?).

Given that, messages like this are a bit discouraging. Rest assured your changes will be reviewed in due time.

craff · 2023-04-14T04:14:50Z

Sorry for the mail showing impatience. Could you just allow the workflow build for all my PRs (or all PR?), I could check for problems myself. Cheers Le 13 avril 2023 12:51:07 GMT-10:00, Antonio Nuno Monteiro ***@***.***> a écrit :

…

> No news ??? This project I maintained by (unpaid) volunteers. Please allow some time before we can review your changes (for free! Isn’t that beautiful?). Given that, messages like this are a bit discouraging. Rest assured your changes will be reviewed in due time. -- Reply to this email directly or view it on GitHub: #106 (comment) You are receiving this because you authored the thread. Message ID: ***@***.***>

craff · 2023-04-14T19:16:35Z

The macos build seems to loop between the ssl_context (which ends successfully) and the ssl_cipher test that never starts.

craff · 2023-04-14T19:19:25Z

Apart from tests seems ok.

craff · 2023-04-18T18:53:21Z

I redid a build from my github, same thing, a loop on macos. Probably not related to this patch? Not sure.

toots

Just left some comments. I don't want to overstep since we delegated this module but I would personally really like to see this work merged.

I've been hoping that OCaml could have a backend server with speed comparable to nginx/node ever since the work on multi-thread started and since seems to finally be bridging that gap!

src/ssl.mli

anmonteiro · 2023-05-04T19:13:09Z

Thanks, @toots. This is still on my list and I'll try to get to it over the weekend.

craff · 2023-05-04T19:17:44Z

Just left some comments. I don't want to overstep since we delegated this module but I would personally really like to see this work merged.

I've been hoping that OCaml could have a backend server with speed comparable to nginx/node ever since the work on multi-thread started and since seems to finally be bridging that gap!

Thanks for you review, I have commited the change. I guess you had a look at simple_httpd?

toots · 2023-05-05T13:46:59Z

Just left some comments. I don't want to overstep since we delegated this module but I would personally really like to see this work merged.
I've been hoping that OCaml could have a backend server with speed comparable to nginx/node ever since the work on multi-thread started and since seems to finally be bridging that gap!

Thanks for you review, I have commited the change. I guess you had a look at simple_httpd?

Not yet but I plan to.

anmonteiro · 2023-05-09T19:12:34Z

Here are some tests with nginx+ssl (as reference) and simple_http+ssl on a 50MB file

number of connection nginx-ssl simple-ssl without PR simple-ssl with PR
5 50.30 36.56 59.33
10 54.95 32.43 60.96
15 54.63 33.81 61.60
20 53.06 33.08 61.57
This PR brings almost a factor 2 and allows to be better that nginx!

What are the units here?

anmonteiro · 2023-05-09T19:19:07Z

src/ssl.ml

-  let domain = Unix.domain_of_sockaddr sockaddr in
-  let sock =
-    Unix.socket domain Unix.SOCK_STREAM 0 in
+module SslReleaseExt = struct


what does SslReleaseExt, SslComExt etc mean?

I can probably guess SslReleaseExt means "not releasing the runtime lock", but what's SslComExt?

Could you add more descriptive names, even if they're longer?

I addressed this by commit cb3cc1f

anmonteiro · 2023-05-09T19:22:41Z

src/ssl.mli

+    capture all exception that requires you to retry later the same operation.
+*)
+
+module type SslCom = sig


we don't need to export the module type in the signature. Perhaps it'd be better to put it in a ssl_intf.ml and refer to it here.

I like to have documlentation in the same .mli file as a .ml. This is not a big mater if it is duplicated in both ssl.ml and ssl.mli ?

We're exposing a module type that has no use. I'd rather keep the API surface area as small as possible.

The module is used, twice:

module SslRelease : SslCom module SslNoRelease : SslCom

And is the only place that gives the type of the main ssl function like connect, read, ẁrite, .... They are available directly as before via the include of SslRelease, I do not see how to remove SslCom. Or maybe I don't get fully what you mean ?

Right, but it doesn't need to be exposed as a module type, creating yet another item in the public API. you can put the module type in a ssl_intf.ml file and reference it in the signature, for example. I think this talks about it https://www.craigfe.io/posts/the-intf-trick

src/ssl.mli

anmonteiro

I added some comments and suggestions that I'd like to see addressed before we move forward.

Additionally, I'd love to see a different PR for the flush error change (3d7445d) so it can be discussed separately. I did not review that part of the code.

craff · 2023-05-10T03:38:21Z

Here are some tests with nginx+ssl (as reference) and simple_http+ssl on a 50MB file
number of connection nginx-ssl simple-ssl without PR simple-ssl with PR
5 50.30 36.56 59.33
10 54.95 32.43 60.96
15 54.63 33.81 61.60
20 53.06 33.08 61.57
This PR brings almost a factor 2 and allows to be better that nginx!

What are the units here?

Request per seconds

craff · 2023-05-10T04:12:45Z

I added some comments and suggestions that I'd like to see addressed before we move forward.

Additionally, I'd love to see a different PR for the flush error change (3d7445d) so it can be discussed separately. I did not review that part of the code.

I reopen PR Fix flush #104 for only fix_flush (and the warning/error for compilation of a test in 5.0)
I rebased this PR on Norelease+flush #106 (meaning Fix flush #104 should be merged first)
I rebased PR Deprecation old tlsv #107 on Norelease+flush #106.

You already reviewed #104 long ago and I just updated a comment.

Co-authored-by: Antonio Nuno Monteiro <anmonteiro@gmail.com>

craff · 2023-05-11T19:56:38Z

This is starting to shape up really nicely. Thanks for your continued work.

Thanks to you for the review!

Co-authored-by: Antonio Nuno Monteiro <anmonteiro@gmail.com>

craff · 2023-05-12T03:08:25Z

I needed 856a5c5 because of a recent merge in master ?

craff · 2023-05-12T03:14:22Z

About the PR for deprecation of old protocol: I will redo it, it is a very small change and the history is too messy for that now with the rebase (git is really messy for merge/rebase compared to darcs ;-). You can wait to review that I do the new one please.

src/ssl.ml

src/ssl.mli

craff · 2023-05-13T20:13:37Z

Many thanks for your work and latest commit.

src/ssl.ml

src/ssl_stubs.c

src/ssl.mli

anmonteiro · 2023-05-13T23:45:31Z

src/ssl_stubs.c

@@ -1377,15 +1377,12 @@ CAMLprim value ocaml_ssl_embed_socket(value socket_, value context)

  if (socket < 0)
    caml_raise_constant(*caml_named_value("ssl_exn_invalid_socket"));
-  caml_release_runtime_system();


I hadn't noticed this before. Do we need a new embed socket function? We should preserve the old behavior for ocaml_ssl_embed_socket.

I first duplicated this function, but looking at the documentation, I found that SSL_new and SSL_set_fd can not block and do not do any communication, so one function not acquiring the runtime is enough (or one acquiring it)

But you are right, a lot of function like ocaml_ssl_get_file_descr, should probably not release the runtime lock neither. But maybe you want this in another PR and we currently keep the previous one.

I just reverted (commit 606c1e5) and will submit an issue now.

Thanks. I think this should be good to go now. I'll do a final check later today and merge it. Thanks!

I just openned a new issue #113, if you could tell me what you think, I might propose a PR very quickly (or not at all if you don't like any of the two ideas ;-)

More generally, there is a lot of unanswered issues on ocaml-ssl, this is not a very good for the image of the project. May be some of them desserve to be closed, other to have at least a short answer. I could actually answer some of them maybe ?

Good point. Some issue gardening is probably in order. Feel free to answer them, of course.

I like your idea for the latest one, I'll answer there.

I will first fix issue #103 (and even output_char), I think this is a serious bug (but probably no one is using them ?). I might look at all the other occurrences of ignore while I am at it.

Many thanks again for your review.

I commented a few issues (only one has no answer now) and probably #31 and #42 can be closed.

craff · 2023-05-17T21:39:03Z

I spotted a odoc problem in ssl.mli fixed by 606c1e5

anmonteiro · 2023-05-23T18:51:13Z

Thank you!

craff · 2023-05-23T20:46:49Z

Antonio Nuno Monteiro writes:

Thank you!

Thanks to you too!

…

— Reply to this email directly, view it on GitHub, or unsubscribe. You are receiving this because you authored the thread.

-- Christophe Raffalli tél: +689 87 23 11 48 web: http://raffalli.eu Ce mail est signé avec pgp (Pièce jointe signature.asc, clef sur https://raffalli.eu/pgp) This mail is signed pgp (Attachment signature.asc, clef on https://raffalli.eu/pgp)

toots · 2023-05-23T23:04:56Z

Awesome y'all, that looks like a great collaboration! Looking forward to testing it!

craff · 2023-05-24T19:54:24Z

I did more work and PRs ... and found/fixed more bugs (2 in my recent PRs, 2 in previous code, 1 in previous code which has already issue #103). When you have time, you should first review #116 (one bug fix) and #118 (4 bugs fixes). I may split #118 in four PRs if you prefer. Cheers, Christophe Antonio Nuno Monteiro writes:

…

Thank you! — Reply to this email directly, view it on GitHub, or unsubscribe. You are receiving this because you authored the thread.

-- Christophe Raffalli tél: +689 87 23 11 48 web: http://raffalli.eu Ce mail est signé avec pgp (Pièce jointe signature.asc, clef sur https://raffalli.eu/pgp) This mail is signed pgp (Attachment signature.asc, clef on https://raffalli.eu/pgp)

CHANGES: - Raise an error when `Ssl.flush` isn't successful (savonet/ocaml-ssl#104, savonet/ocaml-ssl#120) - Add an API-compatible `Ssl.Runtime_lock` module. The functions in this module don't release the OCaml runtime lock. While they don't allow other OCaml threads to run concurrently, they don't perform any copying in the underlying data, leading certain workloads to be faster than their counterparts that release the lock. (savonet/ocaml-ssl#106) - Guarantee `Ssl.output_string` writes the whole string by retrying the operation with unwritten bytes (savonet/ocaml-ssl#103, savonet/ocaml-ssl#116) - Fix calls in C stubs that need to call `ERR_clear_error` before the underlying OpenSSL call (savonet/ocaml-ssl#118) - Add a module `Ssl.Error` to retrieve OpenSSL errors in a structured way (savonet/ocaml-ssl#119) - Deprecate Ssl.{SSLv23,SSLv3,TLSv1,TLSv1_1}, which were were formally deprecated in March 2021 and earlier (savonet/ocaml-ssl#115).

craff added 6 commits January 10, 2023 11:04

raise an exception when flush encounter an error

debd27d

remove a warning building test

c67854b

forgot new exception in .mli

6fd98c1

forgot acquire lock

8c5e205

Thread.Exit does not exist in not so old version of OCAml

365ee6f

Allow compilation on ocaml 5.0 by tolerating Thread.exit () deprecate…

7473e7b

…d in test

craff mentioned this pull request Apr 9, 2023

Deprecation old tlsv #107

Closed

toots reviewed May 4, 2023

View reviewed changes

src/ssl.mli Outdated Show resolved Hide resolved

src/ssl.mli Outdated Show resolved Hide resolved

anmonteiro reviewed May 9, 2023

View reviewed changes

src/ssl.mli Outdated Show resolved Hide resolved

anmonteiro reviewed May 9, 2023

View reviewed changes

Better doc for Fix_flush exception

847d053

craff force-pushed the NOrelease+flush branch from c914a0f to 6826028 Compare May 10, 2023 04:08

Update src/ssl.mli

30d86f6

Co-authored-by: Antonio Nuno Monteiro <anmonteiro@gmail.com>

SslCom -> SslMake

28ad4b5

craff and others added 4 commits May 11, 2023 10:46

Merge branch 'master' into NOrelease+flush

54d0373

Update src/ssl.mli

458810f

Co-authored-by: Antonio Nuno Monteiro <anmonteiro@gmail.com>

new pb with Thread.exit in tests/util.ml ???

856a5c5

Module name: RuntimeLock

8d611e8

Retintroduce the doc un RuntimeLock to allow for editor help

358177a

lower case ident

b5c6966

anmonteiro reviewed May 13, 2023

View reviewed changes

src/ssl.ml Outdated Show resolved Hide resolved

src/ssl.ml Outdated Show resolved Hide resolved

src/ssl.ml Outdated Show resolved Hide resolved

src/ssl.mli Outdated Show resolved Hide resolved

src/ssl.mli Outdated Show resolved Hide resolved

Apply suggestions from code review

2f9810a

anmonteiro reviewed May 13, 2023

View reviewed changes

anmonteiro and others added 4 commits May 13, 2023 16:47

Apply suggestions from code review

57b3132

Update src/ssl_stubs.c

b9c4f84

ambiguous documentation in ssl.mli

62e5b84

revert embed_socket

606c1e5

anmonteiro merged commit 5420c29 into savonet:master May 23, 2023
6 checks passed

anmonteiro added a commit that referenced this pull request May 23, 2023

chore: add changelog entries for #104 and #106

425c669

anmonteiro added a commit that referenced this pull request May 23, 2023

chore: add changelog entries for #104 and #106 (#114)

50b1ced

anmonteiro mentioned this pull request Jun 2, 2023

[new release] ssl (0.6.0) ocaml/opam-repository#23863

Closed

anmonteiro mentioned this pull request Jun 3, 2023

[new release] ssl (0.6.0) ocaml/opam-repository#23869

Merged

Norelease+flush #106

Norelease+flush #106

Conversation

craff commented Apr 9, 2023

craff commented Apr 9, 2023

craff commented Apr 9, 2023

craff commented Apr 9, 2023 • edited

craff commented Apr 13, 2023

anmonteiro commented Apr 13, 2023 • edited

craff commented Apr 14, 2023 via email

craff commented Apr 14, 2023

craff commented Apr 14, 2023

craff commented Apr 18, 2023

toots left a comment

Choose a reason for hiding this comment

anmonteiro commented May 4, 2023

craff commented May 4, 2023

toots commented May 5, 2023

anmonteiro commented May 9, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

anmonteiro left a comment

Choose a reason for hiding this comment

craff commented May 10, 2023

craff commented May 10, 2023

craff commented May 11, 2023

craff commented May 12, 2023

craff commented May 12, 2023

craff commented May 13, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

craff commented May 17, 2023

anmonteiro commented May 23, 2023

craff commented May 23, 2023 via email

toots commented May 23, 2023

craff commented May 24, 2023 via email

craff commented Apr 9, 2023 •

edited

anmonteiro commented Apr 13, 2023 •

edited