Travis has found an error in the IO-WAIT implementation under ECL. I fixed it with b036b50.

There are also failed tests under CCL because its IO-WAIT returns NIL which is treated as timeout by the new version of ENSURE-SSL-FUNCALL. The CCL IO-WAIT also emits CCL::COMMUNICATION-DEADLINE-EXPIRED on deadline. Maybe the timeout should be signalled by IO-WAITs instead of ENSURE-SSL-FUNCALL? I'm waiting for your comments.

@jini-zh , I'm not ready to comment - need to study all that.

@jini-zh, implementation aside, in terms of cl+ssl API, what changes for the user?

@jini-zh, so you are trying to solve the problem of hunchentoot not expiring keep-alive connections?
(I read here that some servers have this timeout as 15 seconds: https://en.wikipedia.org/wiki/HTTP_persistent_connection#HTTP_1.1)

Achieving that via the mix of lisp implementation-dependent, sometimes incomplete, interfaces to sockopt and socket read/write functions, mixed into usocket and cl+ssl doesn't seem reliable.

I'd suggest implementing a background thread in hunchentoot which periodically loops over the keep-alive connections and closes timed-out ones. This would work on all implementations and would be more reliable. cc @hanshuebner

Only that the single-threaded version of hunchentoot will not be fixed by such a thread. Therefore we will do what we can in cl+ssl too. But setting socket timeout on an underlying socket, then wrapping it into an OpenSSL stream and hoping the wrapper stream will inherit the same timeout behavior doesn't seem correct. Probably the timeout configuration should be done via cl+ssl API directly.

@jini-zh, can you show in hunchentoot code how "the thread waits for further requests"? What function do you expect to signal the new TIMEOUT condition?

@avodonosov I would be kind of reluctant to recommend accessing CL+SSL streams from multiple threads at the same time, as I would not trust that CL+SSL and the OpenSSL API would like it if they're being called concurrently. If I were to implement the background, timeout-handling thread, I'd make it close the socket using a direct operating system call and then let the other thread that might be waiting on the CL+SSL socket deal with the hanging read/write system call returning an error. That approach would still require investigating whether a close() on one thread would unblock a read() in another thread, but I would suspect so and I would also trust that to be safe in the sense that CL+SSL must be prepared to deal with read() returning an error or fewer bytes than requested in any case.

In that sense, using socket options instead of implementing the timeout in Lisp is safer as well, and I would generally think that this would be the overall best approach if all that is desired is eventual timeout, and not correct deadline behavior.

@avodonosov,

implementation aside, in terms of cl+ssl API, what changes for the user?

In terms of cl+ssl API, a signal is emitted when the underlying stream times out. The signal is sent through SIGNAL, so if it isn't caught, execution proceeds as before the patch. I don't know what is The Right Thing here, but I think that there should be some way to catch a timeout in the wrapped stream. The approach I took seemed to be the least invasive and backwards compatible.

By the way, in the current master branch, there is some logic regarding deadlines on stream input/output. Does it work? There is no way out of the loop in ENSURE-SSL-FUNCALL except with a condition of some sort. The following code raises a condition of TYPE-ERROR because SB-SYS:WAIT-UNTIL-FD-USABLE eventually gets a negative timeout parameter, but that's too obscure to rely on. However, SSL-STREAM-DEADLINE is not exported, so maybe I'm talking about an unfinished feature here.

#!/usr/bin/sbcl --script

(eval-when (:compile-toplevel :load-toplevel :execute)
  (load #p"~/quicklisp/setup.lisp"))

(eval-when (:compile-toplevel :load-toplevel :execute)
  (ql:quickload "usocket")
  (ql:quickload "cl+ssl"))

(let ((timeout 2))
  (let (key certificate tmp)
    (setf key         (cadr  sb-ext:*posix-argv*)
          certificate (caddr sb-ext:*posix-argv*))
    (unless (and key certificate)
      (setf tmp         (sb-posix:mkdtemp "/tmp/cl+ssl-test.XXXXXX")
            key         (format nil "~a/key" tmp)
            certificate (format nil "~a/certificate" tmp))
      (sb-ext:run-program "openssl"
                          (list "req"
                                "-x509"
                                "-newkey" "rsa:512"
                                "-keyout" key
                                "-out"    certificate
                                "-nodes"
                                "-subj"   "/CN=localhost")
                          :search t
                          :output t))
    (unwind-protect
      (let ((server
              (sb-thread:make-thread
                (lambda ()
                  (usocket:with-server-socket (listener (usocket:socket-listen
                                                          "localhost" 26666))
                    (let ((client (usocket:socket-accept listener)))
                      (let ((ssl (cl+ssl:make-ssl-server-stream
                                   (usocket:socket-stream client)
                                   :certificate certificate
                                   :key key)))
                        ; set the timeout through the cl+ssl deadline API
                        (setf (cl+ssl::ssl-stream-deadline ssl)
                              (coerce (+ (get-internal-real-time)
                                         (* timeout internal-time-units-per-second))
                                      'single-float))
                        (let ((value (read-byte ssl)))
                          (format t "Got ~a~%" value))))))
                :name "server")))
        (sleep 1)
        (usocket:with-client-socket (socket stream "localhost" 26666)
          (let ((ssl (cl+ssl:make-ssl-client-stream stream
                                                    :certificate certificate
                                                    :key key
                                                    :verify nil)))
            (format t "Awaiting timeout (~d seconds)~%" timeout)
            (sb-thread:join-thread server))))
      (when tmp
        (sb-posix:unlink key)
        (sb-posix:unlink certificate)
        (sb-posix:rmdir  tmp)))))

so you are trying to solve the problem of hunchentoot not expiring keep-alive connections?

Yes.

Achieving that via the mix of lisp implementation-dependent, sometimes incomplete, interfaces to sockopt and socket read/write functions, mixed into usocket and cl+ssl doesn't seem reliable.

Well, this is the way the timeout is set in the Hunchentoot code, and also the way the deadlines are implemented in CL+SSL (INPUT-WAIT and OUTPUT-WAIT depend on the implementation).

I don't like the solution with the timeout-handling thread. For one thing, it's going to be tricky to access the thread socket, fd-stream or ssl-stream. Currently the ssl-stream is bound to a Hunchentoot dynamic variable. I will need to manage a list of streams and work around possible race conditions. For the other thing, having a thread messing with other threads just to make them terminate on timeout doesn't feel right. If you won't accept the patch, I will probably just override CL+SSL::INPUT-WAIT in my project.

can you show in hunchentoot code how "the thread waits for further requests"? What function do you expect to signal the new TIMEOUT condition?

When a client connects to the server, generic function HUCHENTOOT:PROCESS-CONNECTION is called with two arguments: ACCEPTOR which is an instance of a subclass of HUNCHENTOOT:ACCEPTOR, and SOCKET which is an instance of USOCKET:STREAM-USOCKET. PROCESS-CONNECTION calls generic function INITIALIZE-CONNECTION-STREAM which wraps SOCKET with a CL+SSL::SSL-STREAM if ACCEPTOR is also an instance of a subclass of HUNCHENTOOT:SSL-ACCEPTOR (in other words, if SSL is enabled). Then PROCESS-CONNECTION processes requests in a loop, blocking while reading from the socket. In SBCL, when SSL is disabled, the reading is performed on an instance of SB-SYS:FD-STREAM. SB-SYS:IO-TIMEOUT is raised on timeout. In the :AROUND method of PROCESS-CONNECTION, it is first converted to USOCKET:TIMEOUT-ERROR by the WITH-MAPPED-CONDITIONS macro and then logged with the WITH-CONDITIONS-CAUGHT-AND-LOGGED macro. After that the thread is allowed to terminate. When SSL is enabled, the timeout is ignored, and the thread is blocked in SB-SYS:WAIT-UNTIL-FD-USABLE called from CL+SSL::INPUT-WAIT until the socket is closed gracefully.

With the patch applied, I expect to get CL+SSL:TIMEOUT in GET-REQUEST-DATA called in PROCESS-CONNECTION. Then I can handle the timeout with the following code:

(defmethod hunchentoot:process-connection ((acceptor ssl-webserver) stream)
  (handler-case (call-next-method)
    (cl+ssl:timeout ())))

where SSL-WEBSERVER is a subclass of HUNCHENTOOT:SSL-WEBSERVER (the symbol belongs to another package).

Given the process with SSL disabled described above, I now think that this is not the proper approach. A better way would be to raise an error in CL+SSL::IO-WAIT: SB-SYS:IO-TIMEOUT in SBCL and CCL::COMMUNICATION-DEADLINE-EXPIRED in CCL (unless there is a better suited condition in CCL, I've got to check its documentation). I think that's the way it is supposed to work with deadlines in CCL. @avodonosov, would you like me to implement it?

@hanshuebner, what is the "correct deadline behavior"?

@jini-zh,

The signal is sent through SIGNAL, so if it isn't caught, execution proceeds as before the patch.

That is a great API.

By the way, in the current master branch, there is some logic regarding deadlines on stream input/output.

Exactly! They seem to be called deadlines. And it is the third part of the equation we need to solve in this issue.

It was implemented in this commit: 5bd5225

I even suspect your goal may be achieved by implementing ssl-stream-deadline for your stream, which would take into account the underlying stream deadline.

Why the timeout not honored today? Because (sb-sys:wait-until-fd-usable ...) is called with nil timeout, where nil is the value of (ssl-stream-deadline stream), right?

So the problem we address here is not when OpenSSL reads the socket, but when we wait with (sb-sys:wait-until-fd-usable ...).

The case of OpenSSL read encoutering timeout deserves attention too. What will ssl-read return if the socke read it calls returns EWOULDBLOCK because of the timeout? How will our ensure-ssl-funcall handle the ssl-read return value?

@avodonosov sorry, I am having a very busy week. I will come back to this topic at the weekend.

@avodonosov

That is a great API.

I know it isn't. I was hoping you'd help me to find a better solution. I didn't want to be intrusive.

I even suspect your goal may be achieved by implementing ssl-stream-deadline for your stream, which would take into account the underlying stream deadline.

I was thinking of a more general approach. In fact, I'm surprised that nobody complained about stale threads with SBCL + Hunchentoot + SSL so far. I suspect that I'm doing something wrong, but it also might the undermanning of the Common Lisp community. Anyway, I found a better solution (see below).

Why the timeout not honored today? Because (sb-sys:wait-until-fd-usable ...) is called with nil timeout, where nil is the value of (ssl-stream-deadline stream), right?

Right.

So the problem we address here is not when OpenSSL reads the socket, but when we wait with (sb-sys:wait-until-fd-usable ...).

Correct.

The case of OpenSSL read encoutering timeout deserves attention too. What will ssl-read return if the socke read it calls returns EWOULDBLOCK because of the timeout? How will our ensure-ssl-funcall handle the ssl-read return value?

SBCL tests whether a read from an FD-STREAM not associated with a regular file would block with a call to poll with zero timeout (see SYSREAD-MAY-BLOCK-P in src/code/fd-stream.lisp; it is called from REFILL-INPUT-BUFFER, the next function in this file), and then uses poll again to wait for input data. poll ignores socket's SO_RCVTIMEO, so the timeout is ignored both with CL+SSL and USOCKET streams. There seems to be no way to set SO_RCVTIMEO on a socket without writing low-level CFFI code.

I found that if UNWRAP-STREAM-P is set to NIL in the call to MAKE-SSL-SERVER-STREAM, then the OpenSSL library is arranged to read from Lisp streams through a BIO struct rather than straight from the file descriptor. In this case the thread blocks in SB-SYS:WAIT-UNTIL-FD-USABLE called from SBCL stream internals rather than CL+SSL::INPUT-WAIT, and the TIMEOUT slot of the FD-STREAM is handled appropriately. This is contrary to CL+SSL documentation on CL+SSL:MAKE-SSL-SERVER-STREAM which says that the deadline will be taken into account when UNWRAP-STREAM-P is true. (By the way, I just noticed that in the documentation, the argument is UNWRAP-STREAMS-P, with plural "streams". You might want to fix that.) That's the only place where the deadlines are mentioned in the documentation, so it isn't clear what they are.

I am going to test that next week. If I get no stale threads, I will submit a patch to Hunchentoot. Would you want me to implement the timeout in CL+SSL anyway?

I'm surprised that nobody complained about stale threads with SBCL + Hunchentoot + SSL so far.

Maybe people implement SSL by putting nginx or another reverse proxy in front of Hunchentoot.

The case of OpenSSL read encoutering timeout deserves attention too. What will ssl-read return if the socke read it calls returns EWOULDBLOCK because of the timeout? How will our ensure-ssl-funcall handle the ssl-read return value?

SBCL tests whether a read from an FD-STREAM not associated with a regular file would...

If that's an answer to my question, then I meant a different thing. Not when lisp code waits for input, but when OpenSSL code reads the file descriptor provided, and encounters the timeout.

This is contrary to CL+SSL documentation on CL+SSL:MAKE-SSL-SERVER-STREAM which says that the deadline will be taken into account when UNWRAP-STREAM-P is true.

There are some deadline examples in https://github.com/cl-plus-ssl/cl-plus-ssl/blob/90619d23f7deca0949ba41f3c3a99921c8643a1f/test.lisp. Maybe that can shed some light on the question.

then the OpenSSL library is arranged to read from Lisp streams through a BIO struct
... I am going to test that next week.

Years ago I heard on the mailing list that lisp BIOs are unstable. That's why people use file descriptor approach with cl+ssl. But maybe that is imprecise or just outdated info. You can try.

(By the way, I just noticed that in the documentation, the argument is UNWRAP-STREAMS-P, with plural "streams". You might want to fix that.)

Fixed, thanks.

Would you want me to implement the timeout in CL+SSL anyway?

I would be glad.

I'm surprised that nobody complained about stale threads with SBCL + Hunchentoot + SSL so far.

Maybe people implement SSL by putting nginx or another reverse proxy in front of Hunchentoot.

Yeah, that looks quite plausible.

The case of OpenSSL read encoutering timeout deserves attention too. What will ssl-read return if the socke read it calls returns EWOULDBLOCK because of the timeout? How will our ensure-ssl-funcall handle the ssl-read return value?

SBCL tests whether a read from an FD-STREAM not associated with a regular file would...

If that's an answer to my question, then I meant a different thing. Not when lisp code waits for input, but when OpenSSL code reads the file descriptor provided, and encounters the timeout.

As per SSL_read man page, SSL returns SSL_ERROR_WANT_READ or SSL_ERROR_WANT_WRITE. I tested it with a C program. If you want to play with it, you may find it here. On the lisp side, ENSURE-SSL-FUNCALL loops according to the error code.

This is contrary to CL+SSL documentation on CL+SSL:MAKE-SSL-SERVER-STREAM which says that the deadline will be taken into account when UNWRAP-STREAM-P is true.

There are some deadline examples in https://github.com/cl-plus-ssl/cl-plus-ssl/blob/90619d23f7deca0949ba41f3c3a99921c8643a1f/test.lisp. Maybe that can shed some light on the question.

Yeah, it can. In READ-DEADLINE I see that the CCL version of the code expects CCL::COMMUNICATION-DEADLINE-EXPIRED which is raised in INPUT-WAIT, while the SBCL version expects SB-SYS:DEADLINE-TIMEOUT which is raised from SBCL internals when the call is wrapped in SB-SYS:WITH-DEADLINE (it is). I noticed that the CCL INPUT-WAIT does use the underlying stream/socket timeout through the call to STREAM-DEADLINE. Apparently, such a functionality was intended in general, but wasn't implemented for SBCL. SB-SYS:WITH-DEADLINE was used instead.

then the OpenSSL library is arranged to read from Lisp streams through a BIO struct ... I am going to test that next week.

Years ago I heard on the mailing list that lisp BIOs are unstable. That's why people use file descriptor approach with cl+ssl. But maybe that is imprecise or just outdated info. You can try.

It has been working perfectly for three days under the load of a few users per hour not counting the bots.

Would you want me to implement the timeout in CL+SSL anyway?

I would be glad.

Cool. I think I now know how to do it. I will just copy the CCL approach. I will also clean up a few things: INPUT-WAIT and OUTPUT-WAIT share too much code. Do you mind if I start afresh with eventual git push -f?

-f is ok

Done. I tested it for a week in production, works fine. I also ran the code in CCL and ECL. All my tests and the tests from fiveam succeeded.

I gave it a thought but couldn't imagine anything important to add. The new behaviour is intuitive and is what I would expect from cl+ssl in the first place. Perhaps you want to make public the new generic functions STREAM-INPUT-TIMEOUT, STREAM-OUTPUT-TIMEOUT and STREAM-DEADLINE? In this case I suggest adding to the documentation the following lines:

Generic function CL+SSL:STREAM-INPUT-TIMEOUT (stream)
Generic function CL+SSL:STREAM-OUTPUT-TIMEOUT (stream)
Generic function CL+SSL:STREAM-DEADLINE (stream) ; Only in Clozure

These functions are used to handle the timeouts and deadlines associated with the stream object passed to cl+ssl:make-ssl-server-stream or cl+ssl:make-ssl-client-stream.

to handle?

@jini-zh , btw, do you use also a hunchentoot patch, or the current hunchentoot works as is with the cl+ssl modification you did?

Is that better?

Generic function CL+SSL:STREAM-INPUT-TIMEOUT (stream)
Generic function CL+SSL:STREAM-OUTPUT-TIMEOUT (stream)
Generic function CL+SSL:STREAM-DEADLINE (stream) ; Only in Clozure

Some Common Lisp implementations support setting a timeout on a stream associated with a socket. These functions are used to extract timeouts and deadlines from the stream object passed to cl+ssl:make-ssl-server-stream or cl+ssl:make-ssl-client-stream. If they return a number, CL+SSL uses it to set up a timeout while waiting for the stream to be able to process a complete chunk of encrypted data. When a timeout is fired, an implementation defined error is raised. Currently supported implementations are SBCL and Clozure. For SBCL, see the section "Synchronous Timeouts (Deadlines)" in the user manual and the documentation on SB-SYS:WAIT-UNTIL-FD-USABLE; for Clozure see the documentation section "Stream Timeouts and Deadlines".

do you use also a hunchentoot patch, or the current hunchentoot works as is with the cl+ssl modification you did?

Yes, I was testing with unpatched Hunchentoot. It works as is.

Yes, better, thanks.

@avodonosov I have exported the functions and updated the documentation.

See if you like this version. I left deadlines support for Clozure.

@jini-zh, sorry for the delay. I'd like to change something, but it's easier to do myyself than asking in the comments. I hope to do that maybe this weekend.

@jini-zh , for you to understand the reason for the delay.

100% backwards compatibility is needed. When I say I'm not going to extend the current deadline functionality, it doesn't mean to drop it. What worked before should work now.

The same for timeouts. The code which didn't have timeouts previously should not get them after the pull request. If the new timeout semantics is needed, this shoud be specified by a new parameter in the make-ssl-(client or server)-stream functions.

Also, I'm trying to rely on the old testcases in the test.lisp file - make them passing now and make sure they are not boken after this change.

And I have very little free time, unfortunately.

@jini-zh, if you rely on Lisp BIO, beware of #72