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

tls-lwt: do not catch out of memory exception #469

Merged
merged 1 commit into from Feb 6, 2023

Conversation

hannesm
Copy link
Member

@hannesm hannesm commented Feb 6, 2023

as discussed in #464 (thanks to @talex5 for raising this)

as discussed in mirleft#464 (thanks to @talex5 for raising this)
@talex5
Copy link
Contributor

talex5 commented Feb 6, 2023

Sorry, that wasn't what I meant.

Marking a flow as broken when you get Out_of_memory is a perfectly fine thing to do - it is very likely to be in a bad state and should not be used again.

Rather, I meant we want to be able to distinguish between:

  • The system is currently out of memory (try dropping some caches), and
  • This flow cannot be used because the system was previously out of memory (close it and open a new one).

Out_of_memory was just an example of an exception that is confusing if re-raised later.

@hannesm
Copy link
Member Author

hannesm commented Feb 6, 2023

Thanks for your comment @talex5, but I think the Out_of_memory exception is really exceptional, and the application should deal with it.There's nothing a library can do -- any further allocation (even for closing streams, etc.) may lead to subsequent failures. Read further on mirage/mirage#1036 if you're interested in why it's bad to catch and hide Out_of_memory.

@hannesm hannesm merged commit 7da3ffd into mirleft:main Feb 6, 2023
@hannesm hannesm deleted the do-not-catch-oom branch February 6, 2023 10:53
@talex5
Copy link
Contributor

talex5 commented Feb 6, 2023

I agree that libraries shouldn't be suppressing Out_of_memory (which the Mirage libraries did), but recording it and immediately raising it again is OK.

What I don't like is having special cases (e.g. what about Stack_overflow or Assert_failure?). The application should always get to see unexpected exceptions.

So, I think TLS should either store all unknown exceptions or none of them, and report it to the application in all cases.

The main tricky case is read, which may successfully read some data but then fail to write. Currently it reports success and then reports the write failure later.

There's nothing a library can do -- any further allocation (even for closing streams, etc.) may lead to subsequent failures.

That's not a big problem. It's responding to an error in a complex operation (e.g. reading TLS data) by trying to perform a simpler one (e.g. marking the flow as failed). If that runs out of memory too, it will still get reported.

hannesm added a commit to hannesm/opam-repository that referenced this pull request Feb 14, 2023
CHANGES:

* BREAKING: new opam package tls-lwt (formerly tls.lwt), in dune:
  (libraries tls.lwt) should now be libraries (tls-lwt)
  (mirleft/ocaml-tls#468 @hannesm, reported mirleft/ocaml-tls#449 by @mbacarella)
* tls: update to mirage-crypto 0.11 API (mirleft/ocaml-tls#468 @hannesm)
* tls: relax SignatureAlgorithms extension handling to allow OpenSSL
  interoperability tests with TLS 1.0 and TLS 1.1 (mirleft/ocaml-tls#469 @hannesm)
* tls: remove Utils.filter_map and and Utils.option, use Stdlib instead (mirleft/ocaml-tls#455
  @hannesm)
* tls: do not globally open Utils (mirleft/ocaml-tls#455 @hannesm)
* tls: export log source of Tracing module (mirleft/ocaml-tls#461 @bikallem)
* tls: remove unused ciphersuites to reduce binary size (mirleft/ocaml-tls#467 @hannesm)
* tls-lwt: do not catch out of memory exception (mirleft/ocaml-tls#469 @hannesm)
* tls-eio: add fuzz testing using crowbar (mirleft/ocaml-tls#456 mirleft/ocaml-tls#463 @talex5)
* tls-eio: update to eio 0.7 (mirleft/ocaml-tls#456 @talex5)
* tls-eio: fix test for develop with vendoring (mirleft/ocaml-tls#462 @bikallem)
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.

None yet

2 participants