savonet / ocaml-ssl

OCaml SSL bindings.
http://liquidsoap.info/ocaml-ssl/
Other
57 stars 48 forks source link

Do not force SSL_MODE_AUTO_RETRY #122

Closed craff closed 2 weeks ago

craff commented 1 year ago

Currently all context have this flag, which is bad in non blocking context with eio or simple_httpd as the scheduler will loose opportunities to switch task.

craff commented 1 year ago

PR #134 should solve this

anmonteiro commented 1 year ago

I don't know if we need to solve this at all. Here's what the documentation says:

SSL_MODE_AUTO_RETRY Never bother the application with retries if the transport is blocking. If a renegotiation take place during normal operation, a SSL_read(3) or SSL_write(3) would return with -1 and indicate the need to retry with SSL_ERROR_WANT_READ. In a non-blocking environment applications must be prepared to handle incomplete read/write operations. In a blocking environment, applications are not always prepared to deal with read/write operations returning without success report. The flag SSL_MODE_AUTO_RETRY will cause read/write operations to only return after the handshake and successful completion.

My reading is that this flag only has effect on blocking sockets. So your non-blocking use case shouldn't be impacted. And I've definitely verified myself in eio-ssl that non-blocking sockets get retry exceptions.

craff commented 1 year ago

Yes, I saw that also. But I think with non blocking we want allow_partial_write in most application ? and we probably want async (but the documentation is really very short for this).

So in the PR #134, I put as default

I want to find more information on async ...

For auto_retry in non blocking setting, the documentation states clearly that we must be prepared to retry anyway, but I don't known for sure if the number of retries is affected by this option. There could be other reason for retry than renegociation ? We could try and count the retry on an example with and without the option. Now that the PR contains an example.

Anyway the PR give the oportunuty to set all mode (except "accept moving buffer", which is mandatory in OCaml if we allocate the buffer in the heap)

craff commented 1 year ago

I forgot to say: I think we want the mode with as much as possible retry to switch context as much as possible.

craff commented 2 weeks ago

I think this can be closed.