search

edicl / drakma

HTTP client written in Common Lisp
http://edicl.github.io/drakma/
249 stars 59 forks source link

Pass the socket stream instead of file descriptor to cl+ssl:make-ssl-client-stream #120

Closed avodonosov closed 1 year ago

avodonosov commented 2 years ago

This allows cl+ssl users to choose between SocketBIO and LispBIO by binding cl+ssl:*default-unwrap-stream-p*, and removes the need for :close-callback - when stream is passed tp cl+ssl, it automatically closes this underlying stream when cl+ssl stream is closed.

gefjon commented 2 years ago

I'm reasonably confident of this, but reassure me: is the default toplevel binding of *default-unwrap-stream-p* t? And does that make this a backwards-compatible change? Will this change break anyone's code?

avodonosov commented 2 years ago

Yes, the default value of *default-unwrap-stream-p* is t. The behaviour remains exactly the same as before the change.

The patch opens new flexibility for users, to chose the non-default cl+ssl mode. 

 (let ((cl+ssl:*default-unwrap-stream-p* nil))
   (drakma:http-request "https://httpbin.org/delay/10" :connection-timeout 2))

On CCL this call will be interrupted by timeout in 2 seconds, instead of waiting for 10 seconds, which happens in default mode. This gives CCL users a workaround for #111.

(Partially by accident in a sense, BTW. Drakma passes the :connection-timeout to the :timeout parameter of usocket:socket-connect, which is documented to work as "socket option SO_RCVTIMEO (read timeout)": https://usocket.common-lisp.dev/api-docs.shtml#socket-connect. So the meaning is closer to the drakma parameter :read-timeout. And it happens so that on CCL usocket really set's up a read timeout the socket stream).

cl+ssl is sensitive to timeouts of the socket stream in the cl+ssl:*default-unwrap-stream-p* nil mode, but not sensitive in the default cl+ssl:*default-unwrap-stream-p* t mode.

avodonosov commented 1 year ago

@gefjon, let's merge it. Without this fix, drakma fails with no work-around on ABCL with newer Java. https://github.com/cl-plus-ssl/cl-plus-ssl/issues/182

gefjon commented 1 year ago

Ah, sorry I forgot about this for so long!