[hunchentoot-devel] 'max-threads' behavior for Hunchentoot
Here are some changes I made to implement 'max-threads' semantics for Hunchentoot. I'd like [some version of] this functionality to be installed into Hunchentoot, so that it is a better behaved multi-threaded web server. A few notes: - The function conditionalized out with #+++potentially-faster-way is meant to be a hint as to how we might refuse the connection without invoking the overhead of accepting the over-the-limit connection. It might be slightly faster, but I don't know if I like the idea of constantly closing and reopening the listener. - 'pooled-thread-per-connection-taskmaster' is just a stub right now, although it provides a useful hook for future work. The idea is to use a thread pool, but on my box, thread creation takes about 250 usec, so it's not clear to me that it's worth the effort, particularly since Bordeaux threads doesn't have the necessary functionality to do it correctly. - 'handle-incoming-connection' on 'one-thread-per-connection-taskmaster' should really try to generate an HTTP 503 error, instead of just closing the connection. I tried several things to make this happen, but nothing seemed to work properly. It seems a shame to have to open the client connection, suck in the whole request, etc etc, just to do this. Is there a better way? Is there some sort of "connection refused" we can do at the socket level? Your review comments and any advice on the third issue would be greatly appreciated. Thanks! --Scott Modified: trunk/qres/lisp/libs/hunchentoot/packages.lisp ============================================================================== --- trunk/qres/lisp/libs/hunchentoot/packages.lisp (original) +++ trunk/qres/lisp/libs/hunchentoot/packages.lisp Thu May 27 10:31:21 2010 @@ -192,7 +192,6 @@ "MIME-TYPE" "NEXT-SESSION-ID" "NO-CACHE" - "ONE-THREAD-PER-CONNECTION-TASKMASTER" "PARAMETER" "PARAMETER-ERROR" "POST-PARAMETER" @@ -250,7 +249,6 @@ "SET-COOKIE" "SET-COOKIE*" "SHUTDOWN" - "SINGLE-THREADED-TASKMASTER" #-:hunchentoot-no-ssl "SSL-ACCEPTOR" "SSL-P" "START" @@ -259,7 +257,12 @@ "STOP" "TASKMASTER" "TASKMASTER-ACCEPTOR" - "URL-DECODE" + "SINGLE-THREADED-TASKMASTER" + "ONE-THREAD-PER-CONNECTION-TASKMASTER" + "POOLED-THREAD-PER-CONNECTION-TASKMASTER" + "INCREMENT-TASKMASTER-THREAD-COUNT" + "DECREMENT-TASKMASTER-THREAD-COUNT" + "URL-DECODE" "URL-ENCODE" "USER-AGENT")) Modified: trunk/qres/lisp/libs/hunchentoot/acceptor.lisp ============================================================================== --- trunk/qres/lisp/libs/hunchentoot/acceptor.lisp (original) +++ trunk/qres/lisp/libs/hunchentoot/acceptor.lisp Thu May 27 10:31:21 2010 @@ -86,7 +86,7 @@ reason to change this to NIL.") (input-chunking-p :initarg :input-chunking-p :accessor acceptor-input-chunking-p - :documentation "A generalized boolean denoting + :documentation "A generalized boolean denoting whether the acceptor may use chunked encoding for input, i.e. when accepting request bodies from the client. The default is T and there's usually no reason to change this to NIL.") @@ -117,8 +117,7 @@ process different from the one where START was called.") #-:lispworks (listen-socket :accessor acceptor-listen-socket - :documentation "The socket listening for incoming -connections.") + :documentation "The socket listening for incoming connections.") (acceptor-shutdown-p :initform nil :accessor acceptor-shutdown-p :documentation "A flag that makes the acceptor @@ -349,9 +348,12 @@ ;; the default is to always answer "no" nil) -;; usocket implementation + +;;; usocket implementation #-:lispworks +(progn + (defmethod start-listening ((acceptor acceptor)) (setf (acceptor-listen-socket acceptor) (usocket:socket-listen (or (acceptor-address acceptor) @@ -361,26 +363,61 @@ :element-type '(unsigned-byte 8))) (values)) -#-:lispworks (defmethod accept-connections ((acceptor acceptor)) (usocket:with-server-socket (listener (acceptor-listen-socket acceptor)) (loop - (when (acceptor-shutdown-p acceptor) - (return)) - (when (usocket:wait-for-input listener :timeout +new-connection-wait-time+) - (handler-case - (when-let (client-connection (usocket:socket-accept listener)) - (set-timeouts client-connection - (acceptor-read-timeout acceptor) - (acceptor-write-timeout acceptor)) - (handle-incoming-connection (acceptor-taskmaster acceptor) - client-connection)) - ;; ignore condition - (usocket:connection-aborted-error ())))))) + (when (acceptor-shutdown-p acceptor) + (return)) + (when (usocket:wait-for-input listener :timeout +new-connection-wait-time+) + (handler-case + (let ((taskmaster (acceptor-taskmaster acceptor))) + (when-let (client-connection (usocket:socket-accept listener)) + (set-timeouts client-connection + (acceptor-read-timeout acceptor) + (acceptor-write-timeout acceptor)) + ;; This will bail if the taskmaster has reached its thread limit + (handle-incoming-connection taskmaster client-connection))) + ;; Ignore the error + (usocket:connection-aborted-error ())))))) + +#+++potentially-faster-way +(defmethod accept-connections ((acceptor acceptor)) + (loop + (usocket:with-server-socket (listener (acceptor-listen-socket acceptor)) + (loop named waiter doing + (when (acceptor-shutdown-p acceptor) + (return-from accept-connections)) + (when (usocket:wait-for-input listener :timeout +new-connection-wait-time+) + (handler-case + (let ((taskmaster (acceptor-taskmaster acceptor))) + ;; Optimization to avoid creating the client connection: + ;; if the taskmaster has reached its thread limit, just close + ;; and reopen the listener socket, and don't even call 'accept' + (when (and (taskmaster-max-threads taskmaster) + (> (taskmaster-thread-count taskmaster) (taskmaster-max-threads taskmaster))) + (when-let (handler (taskmaster-too-many-threads-handler taskmaster)) + (funcall handler taskmaster listener)) + (usocket:socket-close listener) ;close the listener + (setq listener nil) + (start-listening acceptor) ;and start up a new one + (return-from waiter)) + (when-let (client-connection (usocket:socket-accept listener)) + (set-timeouts client-connection + (acceptor-read-timeout acceptor) + (acceptor-write-timeout acceptor)) + ;; This will bail if the taskmaster has reached its thread limit + (handle-incoming-connection taskmaster client-connection))) + ;; Ignore the error + (usocket:connection-aborted-error ()))))))) + +) ;#-:lispworks -;; LispWorks implementation + +;;; LispWorks implementation #+:lispworks +(progn + (defmethod start-listening ((acceptor acceptor)) (multiple-value-bind (listener-process startup-condition) (comm:start-up-server :service (acceptor-port acceptor) @@ -398,8 +435,8 @@ ;; is made :function (lambda (handle) (unless (acceptor-shutdown-p acceptor) - (handle-incoming-connection - (acceptor-taskmaster acceptor) handle))) + (let ((taskmaster (acceptor-taskmaster acceptor))) + (handle-incoming-connection taskmaster client-connection)))) ;; wait until the acceptor was successfully started ;; or an error condition is returned :wait t) @@ -409,11 +446,13 @@ (setf (acceptor-process acceptor) listener-process) (values))) -#+:lispworks (defmethod accept-connections ((acceptor acceptor)) (mp:process-unstop (acceptor-process acceptor)) nil) +) ;#+:lispworks + + (defun list-request-dispatcher (request) "The default request dispatcher which selects a request handler based on a list of individual request dispatchers all of which can Modified: trunk/qres/lisp/libs/hunchentoot/taskmaster.lisp ============================================================================== --- trunk/qres/lisp/libs/hunchentoot/taskmaster.lisp (original) +++ trunk/qres/lisp/libs/hunchentoot/taskmaster.lisp Thu May 27 10:31:21 2010 @@ -62,6 +62,21 @@ might terminate all threads that are currently associated with it. This function is called by the acceptor's STOP method.")) +;; Default method +(defmethod taskmaster-max-threads ((taskmaster taskmaster)) + nil) + +;; Default method +(defmethod taskmaster-thread-count ((taskmaster taskmaster)) + 0) + +(defmethod increment-taskmaster-thread-count ((taskmaster taskmaster)) + nil) + +(defmethod decrement-taskmaster-thread-count ((taskmaster taskmaster)) + nil) + + (defclass single-threaded-taskmaster (taskmaster) () (:documentation "A taskmaster that runs synchronously in the thread @@ -80,25 +95,95 @@ ;; in a single-threaded environment we just call PROCESS-CONNECTION (process-connection (taskmaster-acceptor taskmaster) socket)) + (defclass one-thread-per-connection-taskmaster (taskmaster) (#-:lispworks - (acceptor-process :accessor acceptor-process - :documentation "A process that accepts incoming -connections and hands them off to new processes for request -handling.")) + (acceptor-process + :accessor acceptor-process + :documentation + "A process that accepts incoming connections and hands them off to new processes + for request handling.") + (create-thread-function + :initarg :create-thread-function + :initform 'create-taskmaster-thread + :accessor taskmaster-create-thread-function + :documentation + "Function called to create the handler thread; + takes two arguments, the taskmaster and the socket") + ;; Support for bounding the number of threads we'll create + (max-threads + :type (or integer null) + :initarg :max-threads + :initform nil + :accessor taskmaster-max-threads) + (thread-count + :type integer + :initform 0 + :accessor taskmaster-thread-count) + (thread-count-lock + :initform (bt:make-lock "taskmaster-thread-count") + :accessor taskmaster-thread-count-lock) + (worker-thread-name-format + :type (or string null) + :initarg :worker-thread-name-format + :initform "hunchentoot-worker-~A" + :accessor taskmaster-worker-thread-name-format) + (too-many-threads-handler + :initarg :too-many-threads-handler + :initform nil + :accessor taskmaster-too-many-threads-handler + :documentation + "Function called with two arguments, the taskmaster and the socket, + when too many threads reached, just prior to closing the connection")) + (:default-initargs + :too-many-threads-handler 'log-too-many-threads) (:documentation "A taskmaster that starts one thread for listening -to incoming requests and one thread for each incoming connection. +to incoming requests and one new thread for each incoming connection. +If 'max-threads' is supplied, the number of threads is limited to that. This is the default taskmaster implementation for multi-threaded Lisp implementations.")) -;; usocket implementation +(defmethod increment-taskmaster-thread-count ((taskmaster one-thread-per-connection-taskmaster)) + (when (taskmaster-max-threads taskmaster) + (bt:with-lock-held ((taskmaster-thread-count-lock taskmaster)) + (incf (taskmaster-thread-count taskmaster))))) + +(defmethod decrement-taskmaster-thread-count ((taskmaster one-thread-per-connection-taskmaster)) + (when (taskmaster-max-threads taskmaster) + (bt:with-lock-held ((taskmaster-thread-count-lock taskmaster)) + (decf (taskmaster-thread-count taskmaster))))) + +(defun log-too-many-threads (taskmaster socket) + (declare (ignore socket)) + (let* ((acceptor (taskmaster-acceptor taskmaster)) + (logger (and acceptor (acceptor-message-logger acceptor)))) + (when logger + (funcall logger :warning "Can't handle a new connection, too many threads already")))) + + +;;--- If thread creation is too slow, it would be worth finishing this +;;--- For now, it's just a synonym for 'one-thread-per-connection-taskmaster' +(defclass pooled-thread-per-connection-taskmaster (one-thread-per-connection-taskmaster) + ((create-thread-function + :initarg :create-thread-function + :initform 'create-taskmaster-thread + :accessor taskmaster-create-thread-function + :documentation + "Function called to create the handler thread")) + (:documentation "A taskmaster that starts one thread for listening +to incoming requests and then uses a thread pool for each incoming connection. +If 'max-threads' is supplied, the number of threads is limited to that.")) + + +;;; usocket implementation #-:lispworks +(progn + (defmethod shutdown ((taskmaster taskmaster)) taskmaster) -#-:lispworks (defmethod shutdown ((taskmaster one-thread-per-connection-taskmaster)) ;; just wait until the acceptor process has finished, then return (loop @@ -107,16 +192,39 @@ (sleep 1)) taskmaster) -#-:lispworks (defmethod execute-acceptor ((taskmaster one-thread-per-connection-taskmaster)) (setf (acceptor-process taskmaster) - (bt:make-thread (lambda () - (accept-connections (taskmaster-acceptor taskmaster))) - :name (format nil "Hunchentoot listener \(~A:~A)" - (or (acceptor-address (taskmaster-acceptor taskmaster)) "*") - (acceptor-port (taskmaster-acceptor taskmaster)))))) + (bt:make-thread + (lambda () + (accept-connections (taskmaster-acceptor taskmaster))) + :name (format nil "hunchentoot-listener-~A:~A" + (or (acceptor-address (taskmaster-acceptor taskmaster)) "*") + (acceptor-port (taskmaster-acceptor taskmaster)))))) + +(defmethod handle-incoming-connection ((taskmaster one-thread-per-connection-taskmaster) socket) + ;; Only take lock if necessary + (if (taskmaster-max-threads taskmaster) + (if (< (taskmaster-thread-count taskmaster) (taskmaster-max-threads taskmaster)) + (progn + (increment-taskmaster-thread-count taskmaster) + (funcall (taskmaster-create-thread-function taskmaster) taskmaster socket)) + (progn + (when-let (handler (taskmaster-too-many-threads-handler taskmaster)) + (funcall handler taskmaster socket)) + ;; Just close the socket, which will effectively abort the request + ;;--- It sure would be nice to be able to generate an HTTP 503 error, + ;;--- but I just can't seem to get that to work properly + (usocket:socket-close socket))) + (funcall (taskmaster-create-thread-function taskmaster) taskmaster socket))) + +(defun create-taskmaster-thread (taskmaster socket) + (bt:make-thread + (lambda () + (multiple-value-prog1 + (process-connection (taskmaster-acceptor taskmaster) socket) + (decrement-taskmaster-thread-count taskmaster))) + :name (format nil (taskmaster-worker-thread-name-format taskmaster) (client-as-string socket)))) -#-:lispworks (defun client-as-string (socket) "A helper function which returns the client's address and port as a string and tries to act robustly in the presence of network problems." @@ -127,15 +235,14 @@ (usocket:vector-quad-to-dotted-quad address) port)))) -#-:lispworks -(defmethod handle-incoming-connection ((taskmaster one-thread-per-connection-taskmaster) socket) - (bt:make-thread (lambda () - (process-connection (taskmaster-acceptor taskmaster) socket)) - :name (format nil "Hunchentoot worker \(client: ~A)" (client-as-string socket)))) +) ;#-:lispworks + -;; LispWorks implementation +;;; LispWorks implementation #+:lispworks +(progn + (defmethod shutdown ((taskmaster taskmaster)) (when-let (process (acceptor-process (taskmaster-acceptor taskmaster))) ;; kill the main acceptor process, see LW documentation for @@ -143,20 +250,38 @@ (mp:process-kill process)) taskmaster) -#+:lispworks (defmethod execute-acceptor ((taskmaster one-thread-per-connection-taskmaster)) (accept-connections (taskmaster-acceptor taskmaster))) -#+:lispworks -(defmethod handle-incoming-connection ((taskmaster one-thread-per-connection-taskmaster) handle) +(defmethod handle-incoming-connection ((taskmaster one-thread-per-connection-taskmaster) socket) (incf *worker-counter*) ;; check if we need to perform a global GC (when (and *cleanup-interval* (zerop (mod *worker-counter* *cleanup-interval*))) (when *cleanup-function* (funcall *cleanup-function*))) - (mp:process-run-function (format nil "Hunchentoot worker \(client: ~{~A:~A~})" - (multiple-value-list - (get-peer-address-and-port handle))) - nil #'process-connection - (taskmaster-acceptor taskmaster) handle)) + (if (taskmaster-max-threads taskmaster) + (if (< (taskmaster-thread-count taskmaster) (taskmaster-max-threads taskmaster)) + (progn + (increment-taskmaster-thread-count taskmaster) + (funcall (taskmaster-create-thread-function taskmaster) taskmaster socket)) + ;; With any luck, we never get this far if we've exceeded the thread count + ;; "Good" implementations of 'accept-connections' won't even accept connection requests + (progn + (when-let (handler (taskmaster-too-many-threads-handler taskmaster)) + (funcall handler taskmaster socket)) + (usocket:socket-close socket))) + (funcall (taskmaster-create-thread-function taskmaster) taskmaster socket))) + +(defun create-taskmaster-thread (taskmaster socket) + (flet ((process (taskmaster sock) + (multiple-value-prog1 + (process-connection (taskmaster-acceptor taskmaster) socket) + (decrement-taskmaster-thread-count taskmaster)))) + (mp:process-run-function (format nil "hunchentoot-worker-{~A:~A~})" + (multiple-value-list + (get-peer-address-and-port socket))) + nil #'process taskmaster socket))) + +) ;#+:lispworks +
Hi Scott, first off, thank you for taking the time to improve Hunchentoot and for sending a proposed patch. Please have a look at http://weitz.de/patches.html before submitting your next patch for review. In particular, it makes reviews much easier if there is documentation about what the patch means to do. On Thu, May 27, 2010 at 16:57, Scott McKay <swm@itasoftware.com> wrote:
A few notes: - The function conditionalized out with #+++potentially-faster-way is meant to be a hint as to how we might refuse the connection without invoking the overhead of accepting the over-the-limit connection. It might be slightly faster, but I don't know if I like the idea of constantly closing and reopening the listener.
I don't like the idea, as it opens up a race condition which will result in connections being rejected under high load.
- 'handle-incoming-connection' on 'one-thread-per-connection-taskmaster' should really try to generate an HTTP 503 error, instead of just closing the connection. I tried several things to make this happen, but nothing seemed to work properly. It seems a shame to have to open the client connection, suck in the whole request, etc etc, just to do this. Is there a better way? Is there some sort of "connection refused" we can do at the socket level?
I don't see a need to read the request in order to reply with a 503 error. If the server can't dispatch the request because a resource limit has been hit, there is nothing wrong with just sending a 503 reply without looking at the request at all. Berkeley sockets do not provide a means to reject individual pending connections. Further comments inline:
--Scott
Modified: trunk/qres/lisp/libs/hunchentoot/packages.lisp ============================================================================== --- trunk/qres/lisp/libs/hunchentoot/packages.lisp (original) +++ trunk/qres/lisp/libs/hunchentoot/packages.lisp Thu May 27 10:31:21 2010 @@ -192,7 +192,6 @@ "MIME-TYPE" "NEXT-SESSION-ID" "NO-CACHE" - "ONE-THREAD-PER-CONNECTION-TASKMASTER" "PARAMETER" "PARAMETER-ERROR" "POST-PARAMETER" @@ -250,7 +249,6 @@ "SET-COOKIE" "SET-COOKIE*" "SHUTDOWN" - "SINGLE-THREADED-TASKMASTER" #-:hunchentoot-no-ssl "SSL-ACCEPTOR" "SSL-P" "START" @@ -259,7 +257,12 @@ "STOP" "TASKMASTER" "TASKMASTER-ACCEPTOR" - "URL-DECODE" + "SINGLE-THREADED-TASKMASTER" + "ONE-THREAD-PER-CONNECTION-TASKMASTER" + "POOLED-THREAD-PER-CONNECTION-TASKMASTER" + "INCREMENT-TASKMASTER-THREAD-COUNT" + "DECREMENT-TASKMASTER-THREAD-COUNT" + "URL-DECODE" "URL-ENCODE" "USER-AGENT"))
Modified: trunk/qres/lisp/libs/hunchentoot/acceptor.lisp ============================================================================== --- trunk/qres/lisp/libs/hunchentoot/acceptor.lisp (original) +++ trunk/qres/lisp/libs/hunchentoot/acceptor.lisp Thu May 27 10:31:21 2010 @@ -86,7 +86,7 @@ reason to change this to NIL.") (input-chunking-p :initarg :input-chunking-p :accessor acceptor-input-chunking-p - :documentation "A generalized boolean denoting + :documentation "A generalized boolean denoting whether the acceptor may use chunked encoding for input, i.e. when accepting request bodies from the client. The default is T and there's usually no reason to change this to NIL.") @@ -117,8 +117,7 @@ process different from the one where START was called.") #-:lispworks (listen-socket :accessor acceptor-listen-socket - :documentation "The socket listening for incoming -connections.") + :documentation "The socket listening for incoming connections.") (acceptor-shutdown-p :initform nil :accessor acceptor-shutdown-p :documentation "A flag that makes the acceptor @@ -349,9 +348,12 @@ ;; the default is to always answer "no" nil)
-;; usocket implementation + +;;; usocket implementation
#-:lispworks +(progn
What is this progn needed for?
+ (defmethod start-listening ((acceptor acceptor)) (setf (acceptor-listen-socket acceptor) (usocket:socket-listen (or (acceptor-address acceptor) @@ -361,26 +363,61 @@ :element-type '(unsigned-byte 8))) (values))
-#-:lispworks (defmethod accept-connections ((acceptor acceptor)) (usocket:with-server-socket (listener (acceptor-listen-socket acceptor)) (loop - (when (acceptor-shutdown-p acceptor) - (return)) - (when (usocket:wait-for-input listener :timeout +new-connection-wait-time+) - (handler-case - (when-let (client-connection (usocket:socket-accept listener)) - (set-timeouts client-connection - (acceptor-read-timeout acceptor) - (acceptor-write-timeout acceptor)) - (handle-incoming-connection (acceptor-taskmaster acceptor) - client-connection)) - ;; ignore condition - (usocket:connection-aborted-error ())))))) + (when (acceptor-shutdown-p acceptor) + (return)) + (when (usocket:wait-for-input listener :timeout +new-connection-wait-time+) + (handler-case + (let ((taskmaster (acceptor-taskmaster acceptor))) + (when-let (client-connection (usocket:socket-accept listener)) + (set-timeouts client-connection + (acceptor-read-timeout acceptor) + (acceptor-write-timeout acceptor)) + ;; This will bail if the taskmaster has reached its thread limit + (handle-incoming-connection taskmaster client-connection))) + ;; Ignore the error + (usocket:connection-aborted-error ())))))) + +#+++potentially-faster-way +(defmethod accept-connections ((acceptor acceptor)) + (loop + (usocket:with-server-socket (listener (acceptor-listen-socket acceptor)) + (loop named waiter doing + (when (acceptor-shutdown-p acceptor) + (return-from accept-connections)) + (when (usocket:wait-for-input listener :timeout +new-connection-wait-time+) + (handler-case + (let ((taskmaster (acceptor-taskmaster acceptor))) + ;; Optimization to avoid creating the client connection: + ;; if the taskmaster has reached its thread limit, just close + ;; and reopen the listener socket, and don't even call 'accept' + (when (and (taskmaster-max-threads taskmaster) + (> (taskmaster-thread-count taskmaster) (taskmaster-max-threads taskmaster))) + (when-let (handler (taskmaster-too-many-threads-handler taskmaster)) + (funcall handler taskmaster listener)) + (usocket:socket-close listener) ;close the listener + (setq listener nil) + (start-listening acceptor) ;and start up a new one + (return-from waiter)) + (when-let (client-connection (usocket:socket-accept listener)) + (set-timeouts client-connection + (acceptor-read-timeout acceptor) + (acceptor-write-timeout acceptor)) + ;; This will bail if the taskmaster has reached its thread limit + (handle-incoming-connection taskmaster client-connection))) + ;; Ignore the error + (usocket:connection-aborted-error ()))))))) + +) ;#-:lispworks
-;; LispWorks implementation + +;;; LispWorks implementation
#+:lispworks +(progn +
Don't use progn here. Conditionalize the individual top-level forms. Otherwise, automatic reindentation will screw up the source file.
(defmethod start-listening ((acceptor acceptor)) (multiple-value-bind (listener-process startup-condition) (comm:start-up-server :service (acceptor-port acceptor) @@ -398,8 +435,8 @@ ;; is made :function (lambda (handle) (unless (acceptor-shutdown-p acceptor) - (handle-incoming-connection - (acceptor-taskmaster acceptor) handle))) + (let ((taskmaster (acceptor-taskmaster acceptor))) + (handle-incoming-connection taskmaster client-connection)))) ;; wait until the acceptor was successfully started ;; or an error condition is returned :wait t) @@ -409,11 +446,13 @@ (setf (acceptor-process acceptor) listener-process) (values)))
-#+:lispworks (defmethod accept-connections ((acceptor acceptor)) (mp:process-unstop (acceptor-process acceptor)) nil)
+) ;#+:lispworks + + (defun list-request-dispatcher (request) "The default request dispatcher which selects a request handler based on a list of individual request dispatchers all of which can
Modified: trunk/qres/lisp/libs/hunchentoot/taskmaster.lisp ============================================================================== --- trunk/qres/lisp/libs/hunchentoot/taskmaster.lisp (original) +++ trunk/qres/lisp/libs/hunchentoot/taskmaster.lisp Thu May 27 10:31:21 2010 @@ -62,6 +62,21 @@ might terminate all threads that are currently associated with it. This function is called by the acceptor's STOP method."))
+;; Default method +(defmethod taskmaster-max-threads ((taskmaster taskmaster)) + nil) + +;; Default method +(defmethod taskmaster-thread-count ((taskmaster taskmaster)) + 0) + +(defmethod increment-taskmaster-thread-count ((taskmaster taskmaster)) + nil) + +(defmethod decrement-taskmaster-thread-count ((taskmaster taskmaster)) + nil) + + (defclass single-threaded-taskmaster (taskmaster) () (:documentation "A taskmaster that runs synchronously in the thread @@ -80,25 +95,95 @@ ;; in a single-threaded environment we just call PROCESS-CONNECTION (process-connection (taskmaster-acceptor taskmaster) socket))
+ (defclass one-thread-per-connection-taskmaster (taskmaster) (#-:lispworks - (acceptor-process :accessor acceptor-process - :documentation "A process that accepts incoming -connections and hands them off to new processes for request -handling.")) + (acceptor-process + :accessor acceptor-process + :documentation + "A process that accepts incoming connections and hands them off to new processes + for request handling.") + (create-thread-function + :initarg :create-thread-function + :initform 'create-taskmaster-thread + :accessor taskmaster-create-thread-function + :documentation + "Function called to create the handler thread; + takes two arguments, the taskmaster and the socket") + ;; Support for bounding the number of threads we'll create + (max-threads + :type (or integer null) + :initarg :max-threads + :initform nil + :accessor taskmaster-max-threads) + (thread-count + :type integer + :initform 0 + :accessor taskmaster-thread-count) + (thread-count-lock + :initform (bt:make-lock "taskmaster-thread-count") + :accessor taskmaster-thread-count-lock) + (worker-thread-name-format + :type (or string null) + :initarg :worker-thread-name-format + :initform "hunchentoot-worker-~A" + :accessor taskmaster-worker-thread-name-format) + (too-many-threads-handler + :initarg :too-many-threads-handler + :initform nil + :accessor taskmaster-too-many-threads-handler + :documentation + "Function called with two arguments, the taskmaster and the socket, + when too many threads reached, just prior to closing the connection")) + (:default-initargs + :too-many-threads-handler 'log-too-many-threads) (:documentation "A taskmaster that starts one thread for listening -to incoming requests and one thread for each incoming connection. +to incoming requests and one new thread for each incoming connection. +If 'max-threads' is supplied, the number of threads is limited to that.
Why did you chose to implement create-threads-function and too-many-threads-handler as slots rather than generic functions? The latter seems much more natural to me.
This is the default taskmaster implementation for multi-threaded Lisp implementations."))
-;; usocket implementation +(defmethod increment-taskmaster-thread-count ((taskmaster one-thread-per-connection-taskmaster)) + (when (taskmaster-max-threads taskmaster) + (bt:with-lock-held ((taskmaster-thread-count-lock taskmaster)) + (incf (taskmaster-thread-count taskmaster))))) + +(defmethod decrement-taskmaster-thread-count ((taskmaster one-thread-per-connection-taskmaster)) + (when (taskmaster-max-threads taskmaster) + (bt:with-lock-held ((taskmaster-thread-count-lock taskmaster)) + (decf (taskmaster-thread-count taskmaster))))) + +(defun log-too-many-threads (taskmaster socket) + (declare (ignore socket)) + (let* ((acceptor (taskmaster-acceptor taskmaster)) + (logger (and acceptor (acceptor-message-logger acceptor)))) + (when logger + (funcall logger :warning "Can't handle a new connection, too many threads already")))) + + +;;--- If thread creation is too slow, it would be worth finishing this +;;--- For now, it's just a synonym for 'one-thread-per-connection-taskmaster' +(defclass pooled-thread-per-connection-taskmaster (one-thread-per-connection-taskmaster) + ((create-thread-function + :initarg :create-thread-function + :initform 'create-taskmaster-thread + :accessor taskmaster-create-thread-function + :documentation + "Function called to create the handler thread")) + (:documentation "A taskmaster that starts one thread for listening +to incoming requests and then uses a thread pool for each incoming connection. +If 'max-threads' is supplied, the number of threads is limited to that.")) + + +;;; usocket implementation
#-:lispworks +(progn +
Another top-level progn that should go.
(defmethod shutdown ((taskmaster taskmaster)) taskmaster)
-#-:lispworks (defmethod shutdown ((taskmaster one-thread-per-connection-taskmaster)) ;; just wait until the acceptor process has finished, then return (loop @@ -107,16 +192,39 @@ (sleep 1)) taskmaster)
-#-:lispworks (defmethod execute-acceptor ((taskmaster one-thread-per-connection-taskmaster)) (setf (acceptor-process taskmaster) - (bt:make-thread (lambda () - (accept-connections (taskmaster-acceptor taskmaster))) - :name (format nil "Hunchentoot listener \(~A:~A)" - (or (acceptor-address (taskmaster-acceptor taskmaster)) "*") - (acceptor-port (taskmaster-acceptor taskmaster)))))) + (bt:make-thread + (lambda () + (accept-connections (taskmaster-acceptor taskmaster))) + :name (format nil "hunchentoot-listener-~A:~A" + (or (acceptor-address (taskmaster-acceptor taskmaster)) "*") + (acceptor-port (taskmaster-acceptor taskmaster)))))) + +(defmethod handle-incoming-connection ((taskmaster one-thread-per-connection-taskmaster) socket) + ;; Only take lock if necessary + (if (taskmaster-max-threads taskmaster) + (if (< (taskmaster-thread-count taskmaster) (taskmaster-max-threads taskmaster)) + (progn + (increment-taskmaster-thread-count taskmaster) + (funcall (taskmaster-create-thread-function taskmaster) taskmaster socket)) + (progn + (when-let (handler (taskmaster-too-many-threads-handler taskmaster)) + (funcall handler taskmaster socket)) + ;; Just close the socket, which will effectively abort the request + ;;--- It sure would be nice to be able to generate an HTTP 503 error, + ;;--- but I just can't seem to get that to work properly + (usocket:socket-close socket)))
Please do not use (if .. (progn ..) (progn ..)). Use cond instead or refactor. In this case, I'd think that the maintenance of the thread count could be moved into the generic function that creates the thread, once the callback slot has been replaced by a gf.
+ (funcall (taskmaster-create-thread-function taskmaster) taskmaster socket))) + +(defun create-taskmaster-thread (taskmaster socket) + (bt:make-thread + (lambda () + (multiple-value-prog1 + (process-connection (taskmaster-acceptor taskmaster) socket) + (decrement-taskmaster-thread-count taskmaster))) + :name (format nil (taskmaster-worker-thread-name-format taskmaster) (client-as-string socket))))
-#-:lispworks (defun client-as-string (socket) "A helper function which returns the client's address and port as a string and tries to act robustly in the presence of network problems." @@ -127,15 +235,14 @@ (usocket:vector-quad-to-dotted-quad address) port))))
-#-:lispworks -(defmethod handle-incoming-connection ((taskmaster one-thread-per-connection-taskmaster) socket) - (bt:make-thread (lambda () - (process-connection (taskmaster-acceptor taskmaster) socket)) - :name (format nil "Hunchentoot worker \(client: ~A)" (client-as-string socket)))) +) ;#-:lispworks +
-;; LispWorks implementation +;;; LispWorks implementation
#+:lispworks +(progn +
Another top-level progn (not going to point at those if there are any more, please let them all go).
(defmethod shutdown ((taskmaster taskmaster)) (when-let (process (acceptor-process (taskmaster-acceptor taskmaster))) ;; kill the main acceptor process, see LW documentation for @@ -143,20 +250,38 @@ (mp:process-kill process)) taskmaster)
-#+:lispworks (defmethod execute-acceptor ((taskmaster one-thread-per-connection-taskmaster)) (accept-connections (taskmaster-acceptor taskmaster)))
-#+:lispworks -(defmethod handle-incoming-connection ((taskmaster one-thread-per-connection-taskmaster) handle) +(defmethod handle-incoming-connection ((taskmaster one-thread-per-connection-taskmaster) socket) (incf *worker-counter*) ;; check if we need to perform a global GC (when (and *cleanup-interval* (zerop (mod *worker-counter* *cleanup-interval*))) (when *cleanup-function* (funcall *cleanup-function*))) - (mp:process-run-function (format nil "Hunchentoot worker \(client: ~{~A:~A~})" - (multiple-value-list - (get-peer-address-and-port handle))) - nil #'process-connection - (taskmaster-acceptor taskmaster) handle)) + (if (taskmaster-max-threads taskmaster) + (if (< (taskmaster-thread-count taskmaster) (taskmaster-max-threads taskmaster)) + (progn + (increment-taskmaster-thread-count taskmaster) + (funcall (taskmaster-create-thread-function taskmaster) taskmaster socket)) + ;; With any luck, we never get this far if we've exceeded the thread count + ;; "Good" implementations of 'accept-connections' won't even accept connection requests + (progn + (when-let (handler (taskmaster-too-many-threads-handler taskmaster)) + (funcall handler taskmaster socket)) + (usocket:socket-close socket))) + (funcall (taskmaster-create-thread-function taskmaster) taskmaster socket)))
Another (if ... (progn ..)) that should be improved.
+ +(defun create-taskmaster-thread (taskmaster socket) + (flet ((process (taskmaster sock) + (multiple-value-prog1 + (process-connection (taskmaster-acceptor taskmaster) socket) + (decrement-taskmaster-thread-count taskmaster)))) + (mp:process-run-function (format nil "hunchentoot-worker-{~A:~A~})" + (multiple-value-list + (get-peer-address-and-port socket))) + nil #'process taskmaster socket))) + +) ;#+:lispworks +
_______________________________________________ tbnl-devel site list tbnl-devel@common-lisp.net http://common-lisp.net/mailman/listinfo/tbnl-devel
I'll address all these issues, then send the next set of changes back according to the referenced file. BTW, I chose not to use a generic function for the thread creation function and the too-many-threads handler for what I have to assume is the same reason that the various logger functions are also done as slots: so that you aren't forced to subclass just to provide a couple of functions. If this were Java, I'd say subclassing is the right approach, but since it's Lisp, I think supplying a function is better. After all, that's what first-class functions are for! :-) Thanks! On May 30, 2010, at 5:56 AM, Hans Hübner wrote:
Hi Scott,
first off, thank you for taking the time to improve Hunchentoot and for sending a proposed patch. Please have a look at http://weitz.de/patches.html before submitting your next patch for review. In particular, it makes reviews much easier if there is documentation about what the patch means to do.
On Thu, May 27, 2010 at 16:57, Scott McKay <swm@itasoftware.com> wrote:
A few notes: - The function conditionalized out with #+++potentially-faster-way is meant to be a hint as to how we might refuse the connection without invoking the overhead of accepting the over-the-limit connection. It might be slightly faster, but I don't know if I like the idea of constantly closing and reopening the listener.
I don't like the idea, as it opens up a race condition which will result in connections being rejected under high load.
- 'handle-incoming-connection' on 'one-thread-per-connection-taskmaster' should really try to generate an HTTP 503 error, instead of just closing the connection. I tried several things to make this happen, but nothing seemed to work properly. It seems a shame to have to open the client connection, suck in the whole request, etc etc, just to do this. Is there a better way? Is there some sort of "connection refused" we can do at the socket level?
I don't see a need to read the request in order to reply with a 503 error. If the server can't dispatch the request because a resource limit has been hit, there is nothing wrong with just sending a 503 reply without looking at the request at all. Berkeley sockets do not provide a means to reject individual pending connections.
Further comments inline:
--Scott
Modified: trunk/qres/lisp/libs/hunchentoot/packages.lisp ============================================================================== --- trunk/qres/lisp/libs/hunchentoot/packages.lisp (original) +++ trunk/qres/lisp/libs/hunchentoot/packages.lisp Thu May 27 10:31:21 2010 @@ -192,7 +192,6 @@ "MIME-TYPE" "NEXT-SESSION-ID" "NO-CACHE" - "ONE-THREAD-PER-CONNECTION-TASKMASTER" "PARAMETER" "PARAMETER-ERROR" "POST-PARAMETER" @@ -250,7 +249,6 @@ "SET-COOKIE" "SET-COOKIE*" "SHUTDOWN" - "SINGLE-THREADED-TASKMASTER" #-:hunchentoot-no-ssl "SSL-ACCEPTOR" "SSL-P" "START" @@ -259,7 +257,12 @@ "STOP" "TASKMASTER" "TASKMASTER-ACCEPTOR" - "URL-DECODE" + "SINGLE-THREADED-TASKMASTER" + "ONE-THREAD-PER-CONNECTION-TASKMASTER" + "POOLED-THREAD-PER-CONNECTION-TASKMASTER" + "INCREMENT-TASKMASTER-THREAD-COUNT" + "DECREMENT-TASKMASTER-THREAD-COUNT" + "URL-DECODE" "URL-ENCODE" "USER-AGENT"))
Modified: trunk/qres/lisp/libs/hunchentoot/acceptor.lisp ============================================================================== --- trunk/qres/lisp/libs/hunchentoot/acceptor.lisp (original) +++ trunk/qres/lisp/libs/hunchentoot/acceptor.lisp Thu May 27 10:31:21 2010 @@ -86,7 +86,7 @@ reason to change this to NIL.") (input-chunking-p :initarg :input-chunking-p :accessor acceptor-input-chunking-p - :documentation "A generalized boolean denoting + :documentation "A generalized boolean denoting whether the acceptor may use chunked encoding for input, i.e. when accepting request bodies from the client. The default is T and there's usually no reason to change this to NIL.") @@ -117,8 +117,7 @@ process different from the one where START was called.") #-:lispworks (listen-socket :accessor acceptor-listen-socket - :documentation "The socket listening for incoming -connections.") + :documentation "The socket listening for incoming connections.") (acceptor-shutdown-p :initform nil :accessor acceptor-shutdown-p :documentation "A flag that makes the acceptor @@ -349,9 +348,12 @@ ;; the default is to always answer "no" nil)
-;; usocket implementation + +;;; usocket implementation
#-:lispworks +(progn
What is this progn needed for?
+ (defmethod start-listening ((acceptor acceptor)) (setf (acceptor-listen-socket acceptor) (usocket:socket-listen (or (acceptor-address acceptor) @@ -361,26 +363,61 @@ :element-type '(unsigned-byte 8))) (values))
-#-:lispworks (defmethod accept-connections ((acceptor acceptor)) (usocket:with-server-socket (listener (acceptor-listen-socket acceptor)) (loop - (when (acceptor-shutdown-p acceptor) - (return)) - (when (usocket:wait-for-input listener :timeout +new-connection-wait-time+) - (handler-case - (when-let (client-connection (usocket:socket-accept listener)) - (set-timeouts client-connection - (acceptor-read-timeout acceptor) - (acceptor-write-timeout acceptor)) - (handle-incoming-connection (acceptor-taskmaster acceptor) - client-connection)) - ;; ignore condition - (usocket:connection-aborted-error ())))))) + (when (acceptor-shutdown-p acceptor) + (return)) + (when (usocket:wait-for-input listener :timeout +new-connection-wait-time+) + (handler-case + (let ((taskmaster (acceptor-taskmaster acceptor))) + (when-let (client-connection (usocket:socket-accept listener)) + (set-timeouts client-connection + (acceptor-read-timeout acceptor) + (acceptor-write-timeout acceptor)) + ;; This will bail if the taskmaster has reached its thread limit + (handle-incoming-connection taskmaster client-connection))) + ;; Ignore the error + (usocket:connection-aborted-error ())))))) + +#+++potentially-faster-way +(defmethod accept-connections ((acceptor acceptor)) + (loop + (usocket:with-server-socket (listener (acceptor-listen-socket acceptor)) + (loop named waiter doing + (when (acceptor-shutdown-p acceptor) + (return-from accept-connections)) + (when (usocket:wait-for-input listener :timeout +new-connection-wait-time+) + (handler-case + (let ((taskmaster (acceptor-taskmaster acceptor))) + ;; Optimization to avoid creating the client connection: + ;; if the taskmaster has reached its thread limit, just close + ;; and reopen the listener socket, and don't even call 'accept' + (when (and (taskmaster-max-threads taskmaster) + (> (taskmaster-thread-count taskmaster) (taskmaster-max-threads taskmaster))) + (when-let (handler (taskmaster-too-many-threads-handler taskmaster)) + (funcall handler taskmaster listener)) + (usocket:socket-close listener) ;close the listener + (setq listener nil) + (start-listening acceptor) ;and start up a new one + (return-from waiter)) + (when-let (client-connection (usocket:socket-accept listener)) + (set-timeouts client-connection + (acceptor-read-timeout acceptor) + (acceptor-write-timeout acceptor)) + ;; This will bail if the taskmaster has reached its thread limit + (handle-incoming-connection taskmaster client-connection))) + ;; Ignore the error + (usocket:connection-aborted-error ()))))))) + +) ;#-:lispworks
-;; LispWorks implementation + +;;; LispWorks implementation
#+:lispworks +(progn +
Don't use progn here. Conditionalize the individual top-level forms. Otherwise, automatic reindentation will screw up the source file.
(defmethod start-listening ((acceptor acceptor)) (multiple-value-bind (listener-process startup-condition) (comm:start-up-server :service (acceptor-port acceptor) @@ -398,8 +435,8 @@ ;; is made :function (lambda (handle) (unless (acceptor-shutdown-p acceptor) - (handle-incoming-connection - (acceptor-taskmaster acceptor) handle))) + (let ((taskmaster (acceptor-taskmaster acceptor))) + (handle-incoming-connection taskmaster client-connection)))) ;; wait until the acceptor was successfully started ;; or an error condition is returned :wait t) @@ -409,11 +446,13 @@ (setf (acceptor-process acceptor) listener-process) (values)))
-#+:lispworks (defmethod accept-connections ((acceptor acceptor)) (mp:process-unstop (acceptor-process acceptor)) nil)
+) ;#+:lispworks + + (defun list-request-dispatcher (request) "The default request dispatcher which selects a request handler based on a list of individual request dispatchers all of which can
Modified: trunk/qres/lisp/libs/hunchentoot/taskmaster.lisp ============================================================================== --- trunk/qres/lisp/libs/hunchentoot/taskmaster.lisp (original) +++ trunk/qres/lisp/libs/hunchentoot/taskmaster.lisp Thu May 27 10:31:21 2010 @@ -62,6 +62,21 @@ might terminate all threads that are currently associated with it. This function is called by the acceptor's STOP method."))
+;; Default method +(defmethod taskmaster-max-threads ((taskmaster taskmaster)) + nil) + +;; Default method +(defmethod taskmaster-thread-count ((taskmaster taskmaster)) + 0) + +(defmethod increment-taskmaster-thread-count ((taskmaster taskmaster)) + nil) + +(defmethod decrement-taskmaster-thread-count ((taskmaster taskmaster)) + nil) + + (defclass single-threaded-taskmaster (taskmaster) () (:documentation "A taskmaster that runs synchronously in the thread @@ -80,25 +95,95 @@ ;; in a single-threaded environment we just call PROCESS-CONNECTION (process-connection (taskmaster-acceptor taskmaster) socket))
+ (defclass one-thread-per-connection-taskmaster (taskmaster) (#-:lispworks - (acceptor-process :accessor acceptor-process - :documentation "A process that accepts incoming -connections and hands them off to new processes for request -handling.")) + (acceptor-process + :accessor acceptor-process + :documentation + "A process that accepts incoming connections and hands them off to new processes + for request handling.") + (create-thread-function + :initarg :create-thread-function + :initform 'create-taskmaster-thread + :accessor taskmaster-create-thread-function + :documentation + "Function called to create the handler thread; + takes two arguments, the taskmaster and the socket") + ;; Support for bounding the number of threads we'll create + (max-threads + :type (or integer null) + :initarg :max-threads + :initform nil + :accessor taskmaster-max-threads) + (thread-count + :type integer + :initform 0 + :accessor taskmaster-thread-count) + (thread-count-lock + :initform (bt:make-lock "taskmaster-thread-count") + :accessor taskmaster-thread-count-lock) + (worker-thread-name-format + :type (or string null) + :initarg :worker-thread-name-format + :initform "hunchentoot-worker-~A" + :accessor taskmaster-worker-thread-name-format) + (too-many-threads-handler + :initarg :too-many-threads-handler + :initform nil + :accessor taskmaster-too-many-threads-handler + :documentation + "Function called with two arguments, the taskmaster and the socket, + when too many threads reached, just prior to closing the connection")) + (:default-initargs + :too-many-threads-handler 'log-too-many-threads) (:documentation "A taskmaster that starts one thread for listening -to incoming requests and one thread for each incoming connection. +to incoming requests and one new thread for each incoming connection. +If 'max-threads' is supplied, the number of threads is limited to that.
Why did you chose to implement create-threads-function and too-many-threads-handler as slots rather than generic functions? The latter seems much more natural to me.
This is the default taskmaster implementation for multi-threaded Lisp implementations."))
-;; usocket implementation +(defmethod increment-taskmaster-thread-count ((taskmaster one-thread-per-connection-taskmaster)) + (when (taskmaster-max-threads taskmaster) + (bt:with-lock-held ((taskmaster-thread-count-lock taskmaster)) + (incf (taskmaster-thread-count taskmaster))))) + +(defmethod decrement-taskmaster-thread-count ((taskmaster one-thread-per-connection-taskmaster)) + (when (taskmaster-max-threads taskmaster) + (bt:with-lock-held ((taskmaster-thread-count-lock taskmaster)) + (decf (taskmaster-thread-count taskmaster))))) + +(defun log-too-many-threads (taskmaster socket) + (declare (ignore socket)) + (let* ((acceptor (taskmaster-acceptor taskmaster)) + (logger (and acceptor (acceptor-message-logger acceptor)))) + (when logger + (funcall logger :warning "Can't handle a new connection, too many threads already")))) + + +;;--- If thread creation is too slow, it would be worth finishing this +;;--- For now, it's just a synonym for 'one-thread-per-connection-taskmaster' +(defclass pooled-thread-per-connection-taskmaster (one-thread-per-connection-taskmaster) + ((create-thread-function + :initarg :create-thread-function + :initform 'create-taskmaster-thread + :accessor taskmaster-create-thread-function + :documentation + "Function called to create the handler thread")) + (:documentation "A taskmaster that starts one thread for listening +to incoming requests and then uses a thread pool for each incoming connection. +If 'max-threads' is supplied, the number of threads is limited to that.")) + + +;;; usocket implementation
#-:lispworks +(progn +
Another top-level progn that should go.
(defmethod shutdown ((taskmaster taskmaster)) taskmaster)
-#-:lispworks (defmethod shutdown ((taskmaster one-thread-per-connection-taskmaster)) ;; just wait until the acceptor process has finished, then return (loop @@ -107,16 +192,39 @@ (sleep 1)) taskmaster)
-#-:lispworks (defmethod execute-acceptor ((taskmaster one-thread-per-connection-taskmaster)) (setf (acceptor-process taskmaster) - (bt:make-thread (lambda () - (accept-connections (taskmaster-acceptor taskmaster))) - :name (format nil "Hunchentoot listener \(~A:~A)" - (or (acceptor-address (taskmaster-acceptor taskmaster)) "*") - (acceptor-port (taskmaster-acceptor taskmaster)))))) + (bt:make-thread + (lambda () + (accept-connections (taskmaster-acceptor taskmaster))) + :name (format nil "hunchentoot-listener-~A:~A" + (or (acceptor-address (taskmaster-acceptor taskmaster)) "*") + (acceptor-port (taskmaster-acceptor taskmaster)))))) + +(defmethod handle-incoming-connection ((taskmaster one-thread-per-connection-taskmaster) socket) + ;; Only take lock if necessary + (if (taskmaster-max-threads taskmaster) + (if (< (taskmaster-thread-count taskmaster) (taskmaster-max-threads taskmaster)) + (progn + (increment-taskmaster-thread-count taskmaster) + (funcall (taskmaster-create-thread-function taskmaster) taskmaster socket)) + (progn + (when-let (handler (taskmaster-too-many-threads-handler taskmaster)) + (funcall handler taskmaster socket)) + ;; Just close the socket, which will effectively abort the request + ;;--- It sure would be nice to be able to generate an HTTP 503 error, + ;;--- but I just can't seem to get that to work properly + (usocket:socket-close socket)))
Please do not use (if .. (progn ..) (progn ..)). Use cond instead or refactor. In this case, I'd think that the maintenance of the thread count could be moved into the generic function that creates the thread, once the callback slot has been replaced by a gf.
+ (funcall (taskmaster-create-thread-function taskmaster) taskmaster socket))) + +(defun create-taskmaster-thread (taskmaster socket) + (bt:make-thread + (lambda () + (multiple-value-prog1 + (process-connection (taskmaster-acceptor taskmaster) socket) + (decrement-taskmaster-thread-count taskmaster))) + :name (format nil (taskmaster-worker-thread-name-format taskmaster) (client-as-string socket))))
-#-:lispworks (defun client-as-string (socket) "A helper function which returns the client's address and port as a string and tries to act robustly in the presence of network problems." @@ -127,15 +235,14 @@ (usocket:vector-quad-to-dotted-quad address) port))))
-#-:lispworks -(defmethod handle-incoming-connection ((taskmaster one-thread-per-connection-taskmaster) socket) - (bt:make-thread (lambda () - (process-connection (taskmaster-acceptor taskmaster) socket)) - :name (format nil "Hunchentoot worker \(client: ~A)" (client-as-string socket)))) +) ;#-:lispworks +
-;; LispWorks implementation +;;; LispWorks implementation
#+:lispworks +(progn +
Another top-level progn (not going to point at those if there are any more, please let them all go).
(defmethod shutdown ((taskmaster taskmaster)) (when-let (process (acceptor-process (taskmaster-acceptor taskmaster))) ;; kill the main acceptor process, see LW documentation for @@ -143,20 +250,38 @@ (mp:process-kill process)) taskmaster)
-#+:lispworks (defmethod execute-acceptor ((taskmaster one-thread-per-connection-taskmaster)) (accept-connections (taskmaster-acceptor taskmaster)))
-#+:lispworks -(defmethod handle-incoming-connection ((taskmaster one-thread-per-connection-taskmaster) handle) +(defmethod handle-incoming-connection ((taskmaster one-thread-per-connection-taskmaster) socket) (incf *worker-counter*) ;; check if we need to perform a global GC (when (and *cleanup-interval* (zerop (mod *worker-counter* *cleanup-interval*))) (when *cleanup-function* (funcall *cleanup-function*))) - (mp:process-run-function (format nil "Hunchentoot worker \(client: ~{~A:~A~})" - (multiple-value-list - (get-peer-address-and-port handle))) - nil #'process-connection - (taskmaster-acceptor taskmaster) handle)) + (if (taskmaster-max-threads taskmaster) + (if (< (taskmaster-thread-count taskmaster) (taskmaster-max-threads taskmaster)) + (progn + (increment-taskmaster-thread-count taskmaster) + (funcall (taskmaster-create-thread-function taskmaster) taskmaster socket)) + ;; With any luck, we never get this far if we've exceeded the thread count + ;; "Good" implementations of 'accept-connections' won't even accept connection requests + (progn + (when-let (handler (taskmaster-too-many-threads-handler taskmaster)) + (funcall handler taskmaster socket)) + (usocket:socket-close socket))) + (funcall (taskmaster-create-thread-function taskmaster) taskmaster socket)))
Another (if ... (progn ..)) that should be improved.
+ +(defun create-taskmaster-thread (taskmaster socket) + (flet ((process (taskmaster sock) + (multiple-value-prog1 + (process-connection (taskmaster-acceptor taskmaster) socket) + (decrement-taskmaster-thread-count taskmaster)))) + (mp:process-run-function (format nil "hunchentoot-worker-{~A:~A~})" + (multiple-value-list + (get-peer-address-and-port socket))) + nil #'process taskmaster socket))) + +) ;#+:lispworks +
_______________________________________________ tbnl-devel site list tbnl-devel@common-lisp.net http://common-lisp.net/mailman/listinfo/tbnl-devel
_______________________________________________ tbnl-devel site list tbnl-devel@common-lisp.net http://common-lisp.net/mailman/listinfo/tbnl-devel
Scott, as we have chosen to implement Hunchentoot as object oriented program, using generic functions is the proper way to extend the functionality. I do agree that this is not the only way how to structure Lisp programs and it may also not be the best way according to taste and preference. Please use generic functions in order not to make the next guy wonder why part of Hunchentoot is this way, and part of it is another. Thanks. -Hans On Tue, Jun 1, 2010 at 15:23, Scott McKay <swm@itasoftware.com> wrote:
I'll address all these issues, then send the next set of changes back according to the referenced file.
BTW, I chose not to use a generic function for the thread creation function and the too-many-threads handler for what I have to assume is the same reason that the various logger functions are also done as slots: so that you aren't forced to subclass just to provide a couple of functions. If this were Java, I'd say subclassing is the right approach, but since it's Lisp, I think supplying a function is better. After all, that's what first-class functions are for! :-)
Thanks!
On May 30, 2010, at 5:56 AM, Hans Hübner wrote:
Hi Scott,
first off, thank you for taking the time to improve Hunchentoot and for sending a proposed patch. Please have a look at http://weitz.de/patches.html before submitting your next patch for review. In particular, it makes reviews much easier if there is documentation about what the patch means to do.
On Thu, May 27, 2010 at 16:57, Scott McKay <swm@itasoftware.com> wrote:
A few notes: - The function conditionalized out with #+++potentially-faster-way is meant to be a hint as to how we might refuse the connection without invoking the overhead of accepting the over-the-limit connection. It might be slightly faster, but I don't know if I like the idea of constantly closing and reopening the listener.
I don't like the idea, as it opens up a race condition which will result in connections being rejected under high load.
- 'handle-incoming-connection' on 'one-thread-per-connection-taskmaster' should really try to generate an HTTP 503 error, instead of just closing the connection. I tried several things to make this happen, but nothing seemed to work properly. It seems a shame to have to open the client connection, suck in the whole request, etc etc, just to do this. Is there a better way? Is there some sort of "connection refused" we can do at the socket level?
I don't see a need to read the request in order to reply with a 503 error. If the server can't dispatch the request because a resource limit has been hit, there is nothing wrong with just sending a 503 reply without looking at the request at all. Berkeley sockets do not provide a means to reject individual pending connections.
Further comments inline:
--Scott
Modified: trunk/qres/lisp/libs/hunchentoot/packages.lisp ============================================================================== --- trunk/qres/lisp/libs/hunchentoot/packages.lisp (original) +++ trunk/qres/lisp/libs/hunchentoot/packages.lisp Thu May 27 10:31:21 2010 @@ -192,7 +192,6 @@ "MIME-TYPE" "NEXT-SESSION-ID" "NO-CACHE" - "ONE-THREAD-PER-CONNECTION-TASKMASTER" "PARAMETER" "PARAMETER-ERROR" "POST-PARAMETER" @@ -250,7 +249,6 @@ "SET-COOKIE" "SET-COOKIE*" "SHUTDOWN" - "SINGLE-THREADED-TASKMASTER" #-:hunchentoot-no-ssl "SSL-ACCEPTOR" "SSL-P" "START" @@ -259,7 +257,12 @@ "STOP" "TASKMASTER" "TASKMASTER-ACCEPTOR" - "URL-DECODE" + "SINGLE-THREADED-TASKMASTER" + "ONE-THREAD-PER-CONNECTION-TASKMASTER" + "POOLED-THREAD-PER-CONNECTION-TASKMASTER" + "INCREMENT-TASKMASTER-THREAD-COUNT" + "DECREMENT-TASKMASTER-THREAD-COUNT" + "URL-DECODE" "URL-ENCODE" "USER-AGENT"))
Modified: trunk/qres/lisp/libs/hunchentoot/acceptor.lisp ============================================================================== --- trunk/qres/lisp/libs/hunchentoot/acceptor.lisp (original) +++ trunk/qres/lisp/libs/hunchentoot/acceptor.lisp Thu May 27 10:31:21 2010 @@ -86,7 +86,7 @@ reason to change this to NIL.") (input-chunking-p :initarg :input-chunking-p :accessor acceptor-input-chunking-p - :documentation "A generalized boolean denoting + :documentation "A generalized boolean denoting whether the acceptor may use chunked encoding for input, i.e. when accepting request bodies from the client. The default is T and there's usually no reason to change this to NIL.") @@ -117,8 +117,7 @@ process different from the one where START was called.") #-:lispworks (listen-socket :accessor acceptor-listen-socket - :documentation "The socket listening for incoming -connections.") + :documentation "The socket listening for incoming connections.") (acceptor-shutdown-p :initform nil :accessor acceptor-shutdown-p :documentation "A flag that makes the acceptor @@ -349,9 +348,12 @@ ;; the default is to always answer "no" nil)
-;; usocket implementation + +;;; usocket implementation
#-:lispworks +(progn
What is this progn needed for?
+ (defmethod start-listening ((acceptor acceptor)) (setf (acceptor-listen-socket acceptor) (usocket:socket-listen (or (acceptor-address acceptor) @@ -361,26 +363,61 @@ :element-type '(unsigned-byte 8))) (values))
-#-:lispworks (defmethod accept-connections ((acceptor acceptor)) (usocket:with-server-socket (listener (acceptor-listen-socket acceptor)) (loop - (when (acceptor-shutdown-p acceptor) - (return)) - (when (usocket:wait-for-input listener :timeout +new-connection-wait-time+) - (handler-case - (when-let (client-connection (usocket:socket-accept listener)) - (set-timeouts client-connection - (acceptor-read-timeout acceptor) - (acceptor-write-timeout acceptor)) - (handle-incoming-connection (acceptor-taskmaster acceptor) - client-connection)) - ;; ignore condition - (usocket:connection-aborted-error ())))))) + (when (acceptor-shutdown-p acceptor) + (return)) + (when (usocket:wait-for-input listener :timeout +new-connection-wait-time+) + (handler-case + (let ((taskmaster (acceptor-taskmaster acceptor))) + (when-let (client-connection (usocket:socket-accept listener)) + (set-timeouts client-connection + (acceptor-read-timeout acceptor) + (acceptor-write-timeout acceptor)) + ;; This will bail if the taskmaster has reached its thread limit + (handle-incoming-connection taskmaster client-connection))) + ;; Ignore the error + (usocket:connection-aborted-error ())))))) + +#+++potentially-faster-way +(defmethod accept-connections ((acceptor acceptor)) + (loop + (usocket:with-server-socket (listener (acceptor-listen-socket acceptor)) + (loop named waiter doing + (when (acceptor-shutdown-p acceptor) + (return-from accept-connections)) + (when (usocket:wait-for-input listener :timeout +new-connection-wait-time+) + (handler-case + (let ((taskmaster (acceptor-taskmaster acceptor))) + ;; Optimization to avoid creating the client connection: + ;; if the taskmaster has reached its thread limit, just close + ;; and reopen the listener socket, and don't even call 'accept' + (when (and (taskmaster-max-threads taskmaster) + (> (taskmaster-thread-count taskmaster) (taskmaster-max-threads taskmaster))) + (when-let (handler (taskmaster-too-many-threads-handler taskmaster)) + (funcall handler taskmaster listener)) + (usocket:socket-close listener) ;close the listener + (setq listener nil) + (start-listening acceptor) ;and start up a new one + (return-from waiter)) + (when-let (client-connection (usocket:socket-accept listener)) + (set-timeouts client-connection + (acceptor-read-timeout acceptor) + (acceptor-write-timeout acceptor)) + ;; This will bail if the taskmaster has reached its thread limit + (handle-incoming-connection taskmaster client-connection))) + ;; Ignore the error + (usocket:connection-aborted-error ()))))))) + +) ;#-:lispworks
-;; LispWorks implementation + +;;; LispWorks implementation
#+:lispworks +(progn +
Don't use progn here. Conditionalize the individual top-level forms. Otherwise, automatic reindentation will screw up the source file.
(defmethod start-listening ((acceptor acceptor)) (multiple-value-bind (listener-process startup-condition) (comm:start-up-server :service (acceptor-port acceptor) @@ -398,8 +435,8 @@ ;; is made :function (lambda (handle) (unless (acceptor-shutdown-p acceptor) - (handle-incoming-connection - (acceptor-taskmaster acceptor) handle))) + (let ((taskmaster (acceptor-taskmaster acceptor))) + (handle-incoming-connection taskmaster client-connection)))) ;; wait until the acceptor was successfully started ;; or an error condition is returned :wait t) @@ -409,11 +446,13 @@ (setf (acceptor-process acceptor) listener-process) (values)))
-#+:lispworks (defmethod accept-connections ((acceptor acceptor)) (mp:process-unstop (acceptor-process acceptor)) nil)
+) ;#+:lispworks + + (defun list-request-dispatcher (request) "The default request dispatcher which selects a request handler based on a list of individual request dispatchers all of which can
Modified: trunk/qres/lisp/libs/hunchentoot/taskmaster.lisp ============================================================================== --- trunk/qres/lisp/libs/hunchentoot/taskmaster.lisp (original) +++ trunk/qres/lisp/libs/hunchentoot/taskmaster.lisp Thu May 27 10:31:21 2010 @@ -62,6 +62,21 @@ might terminate all threads that are currently associated with it. This function is called by the acceptor's STOP method."))
+;; Default method +(defmethod taskmaster-max-threads ((taskmaster taskmaster)) + nil) + +;; Default method +(defmethod taskmaster-thread-count ((taskmaster taskmaster)) + 0) + +(defmethod increment-taskmaster-thread-count ((taskmaster taskmaster)) + nil) + +(defmethod decrement-taskmaster-thread-count ((taskmaster taskmaster)) + nil) + + (defclass single-threaded-taskmaster (taskmaster) () (:documentation "A taskmaster that runs synchronously in the thread @@ -80,25 +95,95 @@ ;; in a single-threaded environment we just call PROCESS-CONNECTION (process-connection (taskmaster-acceptor taskmaster) socket))
+ (defclass one-thread-per-connection-taskmaster (taskmaster) (#-:lispworks - (acceptor-process :accessor acceptor-process - :documentation "A process that accepts incoming -connections and hands them off to new processes for request -handling.")) + (acceptor-process + :accessor acceptor-process + :documentation + "A process that accepts incoming connections and hands them off to new processes + for request handling.") + (create-thread-function + :initarg :create-thread-function + :initform 'create-taskmaster-thread + :accessor taskmaster-create-thread-function + :documentation + "Function called to create the handler thread; + takes two arguments, the taskmaster and the socket") + ;; Support for bounding the number of threads we'll create + (max-threads + :type (or integer null) + :initarg :max-threads + :initform nil + :accessor taskmaster-max-threads) + (thread-count + :type integer + :initform 0 + :accessor taskmaster-thread-count) + (thread-count-lock + :initform (bt:make-lock "taskmaster-thread-count") + :accessor taskmaster-thread-count-lock) + (worker-thread-name-format + :type (or string null) + :initarg :worker-thread-name-format + :initform "hunchentoot-worker-~A" + :accessor taskmaster-worker-thread-name-format) + (too-many-threads-handler + :initarg :too-many-threads-handler + :initform nil + :accessor taskmaster-too-many-threads-handler + :documentation + "Function called with two arguments, the taskmaster and the socket, + when too many threads reached, just prior to closing the connection")) + (:default-initargs + :too-many-threads-handler 'log-too-many-threads) (:documentation "A taskmaster that starts one thread for listening -to incoming requests and one thread for each incoming connection. +to incoming requests and one new thread for each incoming connection. +If 'max-threads' is supplied, the number of threads is limited to that.
Why did you chose to implement create-threads-function and too-many-threads-handler as slots rather than generic functions? The latter seems much more natural to me.
This is the default taskmaster implementation for multi-threaded Lisp implementations."))
-;; usocket implementation +(defmethod increment-taskmaster-thread-count ((taskmaster one-thread-per-connection-taskmaster)) + (when (taskmaster-max-threads taskmaster) + (bt:with-lock-held ((taskmaster-thread-count-lock taskmaster)) + (incf (taskmaster-thread-count taskmaster))))) + +(defmethod decrement-taskmaster-thread-count ((taskmaster one-thread-per-connection-taskmaster)) + (when (taskmaster-max-threads taskmaster) + (bt:with-lock-held ((taskmaster-thread-count-lock taskmaster)) + (decf (taskmaster-thread-count taskmaster))))) + +(defun log-too-many-threads (taskmaster socket) + (declare (ignore socket)) + (let* ((acceptor (taskmaster-acceptor taskmaster)) + (logger (and acceptor (acceptor-message-logger acceptor)))) + (when logger + (funcall logger :warning "Can't handle a new connection, too many threads already")))) + + +;;--- If thread creation is too slow, it would be worth finishing this +;;--- For now, it's just a synonym for 'one-thread-per-connection-taskmaster' +(defclass pooled-thread-per-connection-taskmaster (one-thread-per-connection-taskmaster) + ((create-thread-function + :initarg :create-thread-function + :initform 'create-taskmaster-thread + :accessor taskmaster-create-thread-function + :documentation + "Function called to create the handler thread")) + (:documentation "A taskmaster that starts one thread for listening +to incoming requests and then uses a thread pool for each incoming connection. +If 'max-threads' is supplied, the number of threads is limited to that.")) + + +;;; usocket implementation
#-:lispworks +(progn +
Another top-level progn that should go.
(defmethod shutdown ((taskmaster taskmaster)) taskmaster)
-#-:lispworks (defmethod shutdown ((taskmaster one-thread-per-connection-taskmaster)) ;; just wait until the acceptor process has finished, then return (loop @@ -107,16 +192,39 @@ (sleep 1)) taskmaster)
-#-:lispworks (defmethod execute-acceptor ((taskmaster one-thread-per-connection-taskmaster)) (setf (acceptor-process taskmaster) - (bt:make-thread (lambda () - (accept-connections (taskmaster-acceptor taskmaster))) - :name (format nil "Hunchentoot listener \(~A:~A)" - (or (acceptor-address (taskmaster-acceptor taskmaster)) "*") - (acceptor-port (taskmaster-acceptor taskmaster)))))) + (bt:make-thread + (lambda () + (accept-connections (taskmaster-acceptor taskmaster))) + :name (format nil "hunchentoot-listener-~A:~A" + (or (acceptor-address (taskmaster-acceptor taskmaster)) "*") + (acceptor-port (taskmaster-acceptor taskmaster)))))) + +(defmethod handle-incoming-connection ((taskmaster one-thread-per-connection-taskmaster) socket) + ;; Only take lock if necessary + (if (taskmaster-max-threads taskmaster) + (if (< (taskmaster-thread-count taskmaster) (taskmaster-max-threads taskmaster)) + (progn + (increment-taskmaster-thread-count taskmaster) + (funcall (taskmaster-create-thread-function taskmaster) taskmaster socket)) + (progn + (when-let (handler (taskmaster-too-many-threads-handler taskmaster)) + (funcall handler taskmaster socket)) + ;; Just close the socket, which will effectively abort the request + ;;--- It sure would be nice to be able to generate an HTTP 503 error, + ;;--- but I just can't seem to get that to work properly + (usocket:socket-close socket)))
Please do not use (if .. (progn ..) (progn ..)). Use cond instead or refactor. In this case, I'd think that the maintenance of the thread count could be moved into the generic function that creates the thread, once the callback slot has been replaced by a gf.
+ (funcall (taskmaster-create-thread-function taskmaster) taskmaster socket))) + +(defun create-taskmaster-thread (taskmaster socket) + (bt:make-thread + (lambda () + (multiple-value-prog1 + (process-connection (taskmaster-acceptor taskmaster) socket) + (decrement-taskmaster-thread-count taskmaster))) + :name (format nil (taskmaster-worker-thread-name-format taskmaster) (client-as-string socket))))
-#-:lispworks (defun client-as-string (socket) "A helper function which returns the client's address and port as a string and tries to act robustly in the presence of network problems." @@ -127,15 +235,14 @@ (usocket:vector-quad-to-dotted-quad address) port))))
-#-:lispworks -(defmethod handle-incoming-connection ((taskmaster one-thread-per-connection-taskmaster) socket) - (bt:make-thread (lambda () - (process-connection (taskmaster-acceptor taskmaster) socket)) - :name (format nil "Hunchentoot worker \(client: ~A)" (client-as-string socket)))) +) ;#-:lispworks +
-;; LispWorks implementation +;;; LispWorks implementation
#+:lispworks +(progn +
Another top-level progn (not going to point at those if there are any more, please let them all go).
(defmethod shutdown ((taskmaster taskmaster)) (when-let (process (acceptor-process (taskmaster-acceptor taskmaster))) ;; kill the main acceptor process, see LW documentation for @@ -143,20 +250,38 @@ (mp:process-kill process)) taskmaster)
-#+:lispworks (defmethod execute-acceptor ((taskmaster one-thread-per-connection-taskmaster)) (accept-connections (taskmaster-acceptor taskmaster)))
-#+:lispworks -(defmethod handle-incoming-connection ((taskmaster one-thread-per-connection-taskmaster) handle) +(defmethod handle-incoming-connection ((taskmaster one-thread-per-connection-taskmaster) socket) (incf *worker-counter*) ;; check if we need to perform a global GC (when (and *cleanup-interval* (zerop (mod *worker-counter* *cleanup-interval*))) (when *cleanup-function* (funcall *cleanup-function*))) - (mp:process-run-function (format nil "Hunchentoot worker \(client: ~{~A:~A~})" - (multiple-value-list - (get-peer-address-and-port handle))) - nil #'process-connection - (taskmaster-acceptor taskmaster) handle)) + (if (taskmaster-max-threads taskmaster) + (if (< (taskmaster-thread-count taskmaster) (taskmaster-max-threads taskmaster)) + (progn + (increment-taskmaster-thread-count taskmaster) + (funcall (taskmaster-create-thread-function taskmaster) taskmaster socket)) + ;; With any luck, we never get this far if we've exceeded the thread count + ;; "Good" implementations of 'accept-connections' won't even accept connection requests + (progn + (when-let (handler (taskmaster-too-many-threads-handler taskmaster)) + (funcall handler taskmaster socket)) + (usocket:socket-close socket))) + (funcall (taskmaster-create-thread-function taskmaster) taskmaster socket)))
Another (if ... (progn ..)) that should be improved.
+ +(defun create-taskmaster-thread (taskmaster socket) + (flet ((process (taskmaster sock) + (multiple-value-prog1 + (process-connection (taskmaster-acceptor taskmaster) socket) + (decrement-taskmaster-thread-count taskmaster)))) + (mp:process-run-function (format nil "hunchentoot-worker-{~A:~A~})" + (multiple-value-list + (get-peer-address-and-port socket))) + nil #'process taskmaster socket))) + +) ;#+:lispworks +
_______________________________________________ tbnl-devel site list tbnl-devel@common-lisp.net http://common-lisp.net/mailman/listinfo/tbnl-devel
_______________________________________________ tbnl-devel site list tbnl-devel@common-lisp.net http://common-lisp.net/mailman/listinfo/tbnl-devel
_______________________________________________ tbnl-devel site list tbnl-devel@common-lisp.net http://common-lisp.net/mailman/listinfo/tbnl-devel
On Jun 1, 2010, at 9:28 AM, Hans Hübner wrote:
Scott,
as we have chosen to implement Hunchentoot as object oriented program, using generic functions is the proper way to extend the functionality. I do agree that this is not the only way how to structure Lisp programs and it may also not be the best way according to taste and preference. Please use generic functions in order not to make the next guy wonder why part of Hunchentoot is this way, and part of it is another.
Well, hang on... that's one of the reasons I *didn't* use generic functions. Take a look at 'access-logger' and 'message-logger' in the 'acceptor' class; this is exactly the model I followed. If I do the new feature using generic functions, I will be doing exactly what you are warning about: making the next guy wonder why one part is one way and another part is another way.
On Tue, Jun 1, 2010 at 16:15, Scott McKay <swm@itasoftware.com> wrote:
Well, hang on... that's one of the reasons I *didn't* use generic functions. Take a look at 'access-logger' and 'message-logger' in the 'acceptor' class; this is exactly the model I followed. If I do the new feature using generic functions, I will be doing exactly what you are warning about: making the next guy wonder why one part is one way and another part is another way.
That's what you get for growing software: There is always something left over from the first iterations. I have a patch pending that will make the logging functions into generic functions and remove the slots containing the functions. Please help us move there :) Thanks, Hans
On Jun 1, 2010, at 10:24 AM, Hans Hübner wrote:
On Tue, Jun 1, 2010 at 16:15, Scott McKay <swm@itasoftware.com> wrote:
Well, hang on... that's one of the reasons I *didn't* use generic functions. Take a look at 'access-logger' and 'message-logger' in the 'acceptor' class; this is exactly the model I followed. If I do the new feature using generic functions, I will be doing exactly what you are warning about: making the next guy wonder why one part is one way and another part is another way.
That's what you get for growing software: There is always something left over from the first iterations. I have a patch pending that will make the logging functions into generic functions and remove the slots containing the functions. Please help us move there :)
Ha ha ha ha! OK, I accept your appeal for help. :-) I'll get something back to you in the next day or so. Thanks! --Scott
Hans, Hans Hübner wrote:
Hi Scott,
first off, thank you for taking the time to improve Hunchentoot and for sending a proposed patch. Please have a look at http://weitz.de/patches.html before submitting your next patch for review. In particular, it makes reviews much easier if there is documentation about what the patch means to do.
On Thu, May 27, 2010 at 16:57, Scott McKay <swm@itasoftware.com> wrote:
A few notes: - The function conditionalized out with #+++potentially-faster-way is meant to be a hint as to how we might refuse the connection without invoking the overhead of accepting the over-the-limit connection. It might be slightly faster, but I don't know if I like the idea of constantly closing and reopening the listener.
I don't like the idea, as it opens up a race condition which will result in connections being rejected under high load.
I don't understand what race condition you mean. Would you please explain? About rejecting connections: We understand that this behavior would not always be desirable, and I assumed that Scott means it as an option rather than the only possible behavior. This behavior can be useful in a cluster of servers, in which you'd like to tell the load balancer that it should choose another server. Thank you! -- Dan
On Tue, Jun 1, 2010 at 17:04, Daniel Weinreb <dlw@itasoftware.com> wrote:
I don't understand what race condition you mean. Would you please explain?
Rereading the patch, I withdraw my concern. The closing and opening of the socket happens when there is a problem anyway, so it is not too bad if one connection more or less gets dropped. But I think I have trouble understanding what the code really wants to do, a comment describing the intended behavior would be helpful.
About rejecting connections: We understand that this behavior would not always be desirable, and I assumed that Scott means it as an option rather than the only possible behavior. This behavior can be useful in a cluster of servers, in which you'd like to tell the load balancer that it should choose another server.
There should be two configurable modes as to how Hunchentoot handles an incoming connection when no handler thread is available: One would set up things so that the connection is accepted, but a 503 error is sent out right away. The other would suspend accepting connections until a thread is available. The latter mode would make new clients hang (and, if the listener queue runs full, be rejected) until resources are available again. Applications that use a load balancer with multiple backends would use the first mode in order to always get quick responses. Applications that consist of only one server would use the second mode with a suitable backlog configuration so that load peaks can (sometimes) be handled without rejecting client connections. -Hans
On Jun 2, 2010, at 2:25 AM, Hans Hübner wrote:
On Tue, Jun 1, 2010 at 17:04, Daniel Weinreb <dlw@itasoftware.com> wrote:
I don't understand what race condition you mean. Would you please explain?
Rereading the patch, I withdraw my concern. The closing and opening of the socket happens when there is a problem anyway, so it is not too bad if one connection more or less gets dropped. But I think I have trouble understanding what the code really wants to do, a comment describing the intended behavior would be helpful.
About rejecting connections: We understand that this behavior would not always be desirable, and I assumed that Scott means it as an option rather than the only possible behavior. This behavior can be useful in a cluster of servers, in which you'd like to tell the load balancer that it should choose another server.
There should be two configurable modes as to how Hunchentoot handles an incoming connection when no handler thread is available: One would set up things so that the connection is accepted, but a 503 error is sent out right away. The other would suspend accepting connections until a thread is available. The latter mode would make new clients hang (and, if the listener queue runs full, be rejected) until resources are available again.
The latter is a nice idea, but I have no good idea how to implement it portably, given the very limited thread model provided by Bordeaux Threads.
Applications that use a load balancer with multiple backends would use the first mode in order to always get quick responses. Applications that consist of only one server would use the second mode with a suitable backlog configuration so that load peaks can (sometimes) be handled without rejecting client connections.
-Hans
_______________________________________________ tbnl-devel site list tbnl-devel@common-lisp.net http://common-lisp.net/mailman/listinfo/tbnl-devel
On Thu, Jun 3, 2010 at 21:12, Scott McKay <swm@itasoftware.com> wrote:
On Jun 2, 2010, at 2:25 AM, Hans Hübner wrote:
There should be two configurable modes as to how Hunchentoot handles an incoming connection when no handler thread is available: One would set up things so that the connection is accepted, but a 503 error is sent out right away. The other would suspend accepting connections until a thread is available. The latter mode would make new clients hang (and, if the listener queue runs full, be rejected) until resources are available again.
The latter is a nice idea, but I have no good idea how to implement it portably, given the very limited thread model provided by Bordeaux Threads.
One possible approach would be: When the acceptor detects that there is a resource shortage, it waits on a synchronization variable until a thread in the pool is freed instead of looping to the next accept. The wait should be periodically interrupted to check for server shutdown. I believe that this should be relatively straightforward, although it may need some refactoring in the ACCEPT-CONNECTIONS generic function. Maybe it makes sense to add a new generic function ACCEPT-NEXT-CONNECTION that could have an :AROUND method for your pooled acceptor class which would wait for a thread to be available before calling the next method. What do you mean by the limited thread model of BT, how does it hurt? -Hans
On Jun 3, 2010, at 3:32 PM, Hans Hübner wrote:
On Thu, Jun 3, 2010 at 21:12, Scott McKay <swm@itasoftware.com> wrote:
On Jun 2, 2010, at 2:25 AM, Hans Hübner wrote:
There should be two configurable modes as to how Hunchentoot handles an incoming connection when no handler thread is available: One would set up things so that the connection is accepted, but a 503 error is sent out right away. The other would suspend accepting connections until a thread is available. The latter mode would make new clients hang (and, if the listener queue runs full, be rejected) until resources are available again.
The latter is a nice idea, but I have no good idea how to implement it portably, given the very limited thread model provided by Bordeaux Threads.
One possible approach would be: When the acceptor detects that there is a resource shortage, it waits on a synchronization variable until a thread in the pool is freed instead of looping to the next accept. The wait should be periodically interrupted to check for server shutdown. I believe that this should be relatively straightforward, although it may need some refactoring in the ACCEPT-CONNECTIONS generic function. Maybe it makes sense to add a new generic function ACCEPT-NEXT-CONNECTION that could have an :AROUND method for your pooled acceptor class which would wait for a thread to be available before calling the next method.
By the way, this is seeming rather beyond the scope of the original patch. I'll spend another day on this, but I really can't justify spending too much longer on it -- even though it's fun.
What do you mean by the limited thread model of BT, how does it hurt?
My mistake! There is 'condition-wait' and 'condition-notify'.
On Thu, Jun 3, 2010 at 21:52, Scott McKay <swm@itasoftware.com> wrote:
On Jun 3, 2010, at 3:32 PM, Hans Hübner wrote:
One possible approach would be: When the acceptor detects that there is a resource shortage, it waits on a synchronization variable until a thread in the pool is freed instead of looping to the next accept. The wait should be periodically interrupted to check for server shutdown. I believe that this should be relatively straightforward, although it may need some refactoring in the ACCEPT-CONNECTIONS generic function. Maybe it makes sense to add a new generic function ACCEPT-NEXT-CONNECTION that could have an :AROUND method for your pooled acceptor class which would wait for a thread to be available before calling the next method.
By the way, this is seeming rather beyond the scope of the original patch. I'll spend another day on this, but I really can't justify spending too much longer on it -- even though it's fun.
I understand - Thanks for taking some extra time! It'd be very nice if the thread pooling mechanism became generally useful and not require a lot of documentation describing its limitations. Cheers, Hans
On Jun 3, 2010, at 4:28 PM, Hans Hübner wrote:
On Thu, Jun 3, 2010 at 21:52, Scott McKay <swm@itasoftware.com> wrote:
On Jun 3, 2010, at 3:32 PM, Hans Hübner wrote:
One possible approach would be: When the acceptor detects that there is a resource shortage, it waits on a synchronization variable until a thread in the pool is freed instead of looping to the next accept. The wait should be periodically interrupted to check for server shutdown. I believe that this should be relatively straightforward, although it may need some refactoring in the ACCEPT-CONNECTIONS generic function. Maybe it makes sense to add a new generic function ACCEPT-NEXT-CONNECTION that could have an :AROUND method for your pooled acceptor class which would wait for a thread to be available before calling the next method.
By the way, this is seeming rather beyond the scope of the original patch. I'll spend another day on this, but I really can't justify spending too much longer on it -- even though it's fun.
I understand - Thanks for taking some extra time! It'd be very nice if the thread pooling mechanism became generally useful and not require a lot of documentation describing its limitations.
Yep, I'm working on this now. It doesn't look like I'll need to do any refactoring, but I'm not sure how to get 'condition-wait' to let itself be interrupted to poll for server shutdowns...
On Thu, Jun 3, 2010 at 22:58, Scott McKay <swm@itasoftware.com> wrote:
Yep, I'm working on this now. It doesn't look like I'll need to do any refactoring, but I'm not sure how to get 'condition-wait' to let itself be interrupted to poll for server shutdowns...
It could be that you need to use condition-notify for that. -Hans
On Jun 3, 2010, at 4:28 PM, Hans Hübner wrote:
On Thu, Jun 3, 2010 at 21:52, Scott McKay <swm@itasoftware.com> wrote:
On Jun 3, 2010, at 3:32 PM, Hans Hübner wrote:
One possible approach would be: When the acceptor detects that there is a resource shortage, it waits on a synchronization variable until a thread in the pool is freed instead of looping to the next accept. The wait should be periodically interrupted to check for server shutdown. I believe that this should be relatively straightforward, although it may need some refactoring in the ACCEPT-CONNECTIONS generic function. Maybe it makes sense to add a new generic function ACCEPT-NEXT-CONNECTION that could have an :AROUND method for your pooled acceptor class which would wait for a thread to be available before calling the next method.
By the way, this is seeming rather beyond the scope of the original patch. I'll spend another day on this, but I really can't justify spending too much longer on it -- even though it's fun.
I understand - Thanks for taking some extra time! It'd be very nice if the thread pooling mechanism became generally useful and not require a lot of documentation describing its limitations.
OK, I seem to have something useful working. Note that the pooled-thread taskmaster is still a stub, for two reasons: 1. Thread creation is actually pretty damned fast (on Linux, anyway -- around 200 microseconds on my box), and it's not clear that it's worth it. 2. This *is* where Bordeaux Threads falls down, maybe because not all Lisp implementations support it. "Restarting" an existing thread seems not to be a supported operation, so we'd need some complex -- and potentially fragile and error-prone -- protocol for doing "restartable" threads. The other stuff now works, and there's a test suite for it all in QRes (sadly, not in H'toot itself).
On Fri, Jun 4, 2010 at 17:08, Scott McKay <swm@itasoftware.com> wrote:
OK, I seem to have something useful working. Note that the pooled-thread taskmaster is still a stub, for two reasons: 1. Thread creation is actually pretty damned fast (on Linux, anyway -- around 200 microseconds on my box), and it's not clear that it's worth it. 2. This *is* where Bordeaux Threads falls down, maybe because not all Lisp implementations support it. "Restarting" an existing thread seems not to be a supported operation, so we'd need some complex -- and potentially fragile and error-prone -- protocol for doing "restartable" threads.
I'd say that the idea of a thread pool can be abandoned if it is not useful, and it seems that it would not be useful. Let's not put in stubs. -Hans
Scott, just as a clarification: I think that if thread pools are not useful (which seems to be the case), we should not have stubs in there for them. I do think that your change, which allows limiting the number of client threads that Hunchentoot has active at any one time, should have the two discussed modes when the limit has been reached: The default mode would be to pause accepting requests or, once the queue runs full, to reject them. The alternative mode would be to accept all new connections but reply with a 503. I don't know what the state of your patch is now, but I guess that the above is what you have implemented. This is just to verify that we're on the same page. Thanks! Hans
On Jun 5, 2010, at 4:26 AM, Hans Hübner wrote:
Scott,
just as a clarification: I think that if thread pools are not useful (which seems to be the case), we should not have stubs in there for them. I do think that your change, which allows limiting the number of client threads that Hunchentoot has active at any one time, should have the two discussed modes when the limit has been reached: The default mode would be to pause accepting requests or, once the queue runs full, to reject them. The alternative mode would be to accept all new connections but reply with a 503.
I don't know what the state of your patch is now, but I guess that the above is what you have implemented. This is just to verify that we're on the same page.
Yes, the above is what I have implemented. I'll remove the "pooled thread" stub on Monday (too busy this weekend). Right now, the default for 'one-thread-per-request-taskmaster' (or whatever it's called) is what it used to be: just keep accepting connections. If you'd like me to change that to "accept N connections and spin off a thread; queue up a few more connections; then issue HTTP 503", that would suit me just fine. It seems like a good, default behavior. The one question I have is, what should N be and what should the increment be beyond which we reject? I'm thinking something like allow 8 threads, and 2-4 on the queue, but I'll go with whatever you think is reasonable.
On Sat, Jun 5, 2010 at 15:01, Scott McKay <swm@itasoftware.com> wrote:
Right now, the default for 'one-thread-per-request-taskmaster' (or whatever it's called) is what it used to be: just keep accepting connections. If you'd like me to change that to "accept N connections and spin off a thread; queue up a few more connections; then issue HTTP 503", that would suit me just fine. It seems like a good, default behavior. The one question I have is, what should N be and what should the increment be beyond which we reject? I'm thinking something like allow 8 threads, and 2-4 on the queue, but I'll go with whatever you think is reasonable.
The default limit for threads needs to be much larger than 8 as - correct me if I'm wrong - there is one thread per connection, not per request. This means that the number of threads allowed is basically identical to the number of simultaneous clients. Lets set the default limit to 100 so that Hunchentoot is not the first thing one needs to tune when coping with larger loads. The listener queue length should not be too short (something like 30-50) so that load transients can be handled. Enjoy your weekend, Hans
On 06/05/2010 09:29 AM, Hans Hübner wrote:
Lets set the default limit to 100 so that Hunchentoot is not the first thing one needs to tune when coping with larger loads. The listener queue length should not be too short (something like 30-50) so that load transients can be handled.
Hans is correct that it should higher. Just for comparison, Tomcat's defaults are about double: 200 threads (maxThreads) and 100 queue size (acceptCount). http://tomcat.apache.org/tomcat-6.0-doc/config/http.html -Gordon
Hans, I've attached a file with the diffs against (what I believe is) the latest version of Hunchentoot. This does what we discussed. On Jun 5, 2010, at 9:29 AM, Hans Hübner wrote:
On Sat, Jun 5, 2010 at 15:01, Scott McKay <swm@itasoftware.com> wrote:
Right now, the default for 'one-thread-per-request-taskmaster' (or whatever it's called) is what it used to be: just keep accepting connections. If you'd like me to change that to "accept N connections and spin off a thread; queue up a few more connections; then issue HTTP 503", that would suit me just fine. It seems like a good, default behavior. The one question I have is, what should N be and what should the increment be beyond which we reject? I'm thinking something like allow 8 threads, and 2-4 on the queue, but I'll go with whatever you think is reasonable.
The default limit for threads needs to be much larger than 8 as - correct me if I'm wrong - there is one thread per connection, not per request. This means that the number of threads allowed is basically identical to the number of simultaneous clients. Lets set the default limit to 100 so that Hunchentoot is not the first thing one needs to tune when coping with larger loads. The listener queue length should not be too short (something like 30-50) so that load transients can be handled.
Enjoy your weekend, Hans
_______________________________________________ tbnl-devel site list tbnl-devel@common-lisp.net http://common-lisp.net/mailman/listinfo/tbnl-devel
Scott, thank you for the patch. May I ask you to add a documentation update? Thanks, Hans On Mon, Jun 7, 2010 at 21:14, Scott McKay <swm@itasoftware.com> wrote:
Hans, I've attached a file with the diffs against (what I believe is) the latest version of Hunchentoot. This does what we discussed.
On Jun 5, 2010, at 9:29 AM, Hans Hübner wrote:
On Sat, Jun 5, 2010 at 15:01, Scott McKay <swm@itasoftware.com> wrote:
Right now, the default for 'one-thread-per-request-taskmaster' (or whatever it's called) is what it used to be: just keep accepting connections. If you'd like me to change that to "accept N connections and spin off a thread; queue up a few more connections; then issue HTTP 503", that would suit me just fine. It seems like a good, default behavior. The one question I have is, what should N be and what should the increment be beyond which we reject? I'm thinking something like allow 8 threads, and 2-4 on the queue, but I'll go with whatever you think is reasonable.
The default limit for threads needs to be much larger than 8 as - correct me if I'm wrong - there is one thread per connection, not per request. This means that the number of threads allowed is basically identical to the number of simultaneous clients. Lets set the default limit to 100 so that Hunchentoot is not the first thing one needs to tune when coping with larger loads. The listener queue length should not be too short (something like 30-50) so that load transients can be handled.
Enjoy your weekend, Hans
_______________________________________________ tbnl-devel site list tbnl-devel@common-lisp.net http://common-lisp.net/mailman/listinfo/tbnl-devel
_______________________________________________ tbnl-devel site list tbnl-devel@common-lisp.net http://common-lisp.net/mailman/listinfo/tbnl-devel
In the file index.xml, I presume? Sure, no problem. Just don't ask me to document all the stuff that was already in the code, but not yet documented! :-) On Jun 7, 2010, at 3:33 PM, Hans Hübner wrote:
Scott,
thank you for the patch. May I ask you to add a documentation update?
Thanks, Hans
On Mon, Jun 7, 2010 at 21:14, Scott McKay <swm@itasoftware.com> wrote:
Hans, I've attached a file with the diffs against (what I believe is) the latest version of Hunchentoot. This does what we discussed.
On Jun 5, 2010, at 9:29 AM, Hans Hübner wrote:
On Sat, Jun 5, 2010 at 15:01, Scott McKay <swm@itasoftware.com> wrote:
Right now, the default for 'one-thread-per-request-taskmaster' (or whatever it's called) is what it used to be: just keep accepting connections. If you'd like me to change that to "accept N connections and spin off a thread; queue up a few more connections; then issue HTTP 503", that would suit me just fine. It seems like a good, default behavior. The one question I have is, what should N be and what should the increment be beyond which we reject? I'm thinking something like allow 8 threads, and 2-4 on the queue, but I'll go with whatever you think is reasonable.
The default limit for threads needs to be much larger than 8 as - correct me if I'm wrong - there is one thread per connection, not per request. This means that the number of threads allowed is basically identical to the number of simultaneous clients. Lets set the default limit to 100 so that Hunchentoot is not the first thing one needs to tune when coping with larger loads. The listener queue length should not be too short (something like 30-50) so that load transients can be handled.
Enjoy your weekend, Hans
_______________________________________________ tbnl-devel site list tbnl-devel@common-lisp.net http://common-lisp.net/mailman/listinfo/tbnl-devel
_______________________________________________ tbnl-devel site list tbnl-devel@common-lisp.net http://common-lisp.net/mailman/listinfo/tbnl-devel
_______________________________________________ tbnl-devel site list tbnl-devel@common-lisp.net http://common-lisp.net/mailman/listinfo/tbnl-devel
On Mon, Jun 7, 2010 at 21:38, Scott McKay <swm@itasoftware.com> wrote:
In the file index.xml, I presume?
Yes please.
Sure, no problem. Just don't ask me to document all the stuff that was already in the code, but not yet documented! :-)
Surely not. Just document the new feature. Thank you! -Hans
On Jun 7, 2010, at 3:33 PM, Hans Hübner wrote:
Scott,
thank you for the patch. May I ask you to add a documentation update?
Thanks, Hans
On Mon, Jun 7, 2010 at 21:14, Scott McKay <swm@itasoftware.com> wrote:
Hans, I've attached a file with the diffs against (what I believe is) the latest version of Hunchentoot. This does what we discussed.
On Jun 5, 2010, at 9:29 AM, Hans Hübner wrote:
On Sat, Jun 5, 2010 at 15:01, Scott McKay <swm@itasoftware.com> wrote:
Right now, the default for 'one-thread-per-request-taskmaster' (or whatever it's called) is what it used to be: just keep accepting connections. If you'd like me to change that to "accept N connections and spin off a thread; queue up a few more connections; then issue HTTP 503", that would suit me just fine. It seems like a good, default behavior. The one question I have is, what should N be and what should the increment be beyond which we reject? I'm thinking something like allow 8 threads, and 2-4 on the queue, but I'll go with whatever you think is reasonable.
The default limit for threads needs to be much larger than 8 as - correct me if I'm wrong - there is one thread per connection, not per request. This means that the number of threads allowed is basically identical to the number of simultaneous clients. Lets set the default limit to 100 so that Hunchentoot is not the first thing one needs to tune when coping with larger loads. The listener queue length should not be too short (something like 30-50) so that load transients can be handled.
Enjoy your weekend, Hans
_______________________________________________ tbnl-devel site list tbnl-devel@common-lisp.net http://common-lisp.net/mailman/listinfo/tbnl-devel
_______________________________________________ tbnl-devel site list tbnl-devel@common-lisp.net http://common-lisp.net/mailman/listinfo/tbnl-devel
_______________________________________________ tbnl-devel site list tbnl-devel@common-lisp.net http://common-lisp.net/mailman/listinfo/tbnl-devel
_______________________________________________ tbnl-devel site list tbnl-devel@common-lisp.net http://common-lisp.net/mailman/listinfo/tbnl-devel
Dear Hunchentoot maintainers, I've merged Scott's changes with the latest upstream hunchentoot from darcs (version 1.1.0 from 2010-01-09) and my XCVB changes. Here it is attached. Can you either apply or tell me how I may change the thing to fit your standards? [ François-René ÐVB Rideau | Reflection&Cybernethics | http://fare.tunes.org ] Too many people are thinking of security instead of opportunity. They seem more afraid of life than death. — James F. Byrnes
Hi Faré, thank you for consolidating the patch. Please resubmit without the xcvb changes (we discussed this before) and make sure that you are submitting a patch against the subversion repository, as we do not use the darcs repository and do not know whether it is up to date or diverged. As a general note: It is much easier to review patches if they do not contain arbitary whitespace changes. Thanks, Hans On Wed, Jul 7, 2010 at 22:51, Faré <fahree@gmail.com> wrote:
Dear Hunchentoot maintainers,
I've merged Scott's changes with the latest upstream hunchentoot from darcs (version 1.1.0 from 2010-01-09) and my XCVB changes.
Here it is attached.
Can you either apply or tell me how I may change the thing to fit your standards?
[ François-René ÐVB Rideau | Reflection&Cybernethics | http://fare.tunes.org ] Too many people are thinking of security instead of opportunity. They seem more afraid of life than death. — James F. Byrnes
_______________________________________________ tbnl-devel site list tbnl-devel@common-lisp.net http://common-lisp.net/mailman/listinfo/tbnl-devel
Hi Faré, The patch standards you asked for are here: http://weitz.de/patches.html And the Subversion repository Hans mentioned is here: http://bknr.net/trac/browser/trunk/thirdparty/hunchentoot Thanks, Edi. On Wed, Jul 7, 2010 at 11:32 PM, Hans Hübner <hans.huebner@gmail.com> wrote:
Hi Faré,
thank you for consolidating the patch. Please resubmit without the xcvb changes (we discussed this before) and make sure that you are submitting a patch against the subversion repository, as we do not use the darcs repository and do not know whether it is up to date or diverged.
As a general note: It is much easier to review patches if they do not contain arbitary whitespace changes.
Thanks, Hans
On Wed, Jul 7, 2010 at 22:51, Faré <fahree@gmail.com> wrote:
Dear Hunchentoot maintainers,
I've merged Scott's changes with the latest upstream hunchentoot from darcs (version 1.1.0 from 2010-01-09) and my XCVB changes.
Here it is attached.
Can you either apply or tell me how I may change the thing to fit your standards?
[ François-René ÐVB Rideau | Reflection&Cybernethics | http://fare.tunes.org ] Too many people are thinking of security instead of opportunity. They seem more afraid of life than death. — James F. Byrnes
_______________________________________________ tbnl-devel site list tbnl-devel@common-lisp.net http://common-lisp.net/mailman/listinfo/tbnl-devel
_______________________________________________ tbnl-devel site list tbnl-devel@common-lisp.net http://common-lisp.net/mailman/listinfo/tbnl-devel
Will do. I see no mention of this repository on the web page for the project: http://weitz.de/hunchentoot/ and it looked like that darcs repository was the closest thing to an upstream, but it might only have recorded public numbered releases. Is the bknr thirdparty svn now the official upstream for all ediware projects? Or at least the official "we're as close to Edi's disk as is otherwise published" mirror? i.e. is that also where I should be getting chunga, drakma, flexi-streams, cl-ppcre, cl-fad, cl-who, etc.? Are there also other software of which you are de facto maintainer, or should I fetch dependencies from their respective upstreams? Thanks a lot for your time! [ François-René ÐVB Rideau | Reflection&Cybernethics | http://fare.tunes.org ] No woman ever falls in love with a man unless she has a better opinion of him than he deserves. — Edgar Watson Howe On 7 July 2010 17:38, Edi Weitz <edi@agharta.de> wrote:
Hi Faré,
The patch standards you asked for are here:
And the Subversion repository Hans mentioned is here:
http://bknr.net/trac/browser/trunk/thirdparty/hunchentoot
Thanks, Edi.
On Wed, Jul 7, 2010 at 11:32 PM, Hans Hübner <hans.huebner@gmail.com> wrote:
Hi Faré,
thank you for consolidating the patch. Please resubmit without the xcvb changes (we discussed this before) and make sure that you are submitting a patch against the subversion repository, as we do not use the darcs repository and do not know whether it is up to date or diverged.
As a general note: It is much easier to review patches if they do not contain arbitary whitespace changes.
Thanks, Hans
On Wed, Jul 7, 2010 at 22:51, Faré <fahree@gmail.com> wrote:
Dear Hunchentoot maintainers,
I've merged Scott's changes with the latest upstream hunchentoot from darcs (version 1.1.0 from 2010-01-09) and my XCVB changes.
Here it is attached.
Can you either apply or tell me how I may change the thing to fit your standards?
[ François-René ÐVB Rideau | Reflection&Cybernethics | http://fare.tunes.org ] Too many people are thinking of security instead of opportunity. They seem more afraid of life than death. — James F. Byrnes
_______________________________________________ tbnl-devel site list tbnl-devel@common-lisp.net http://common-lisp.net/mailman/listinfo/tbnl-devel
_______________________________________________ tbnl-devel site list tbnl-devel@common-lisp.net http://common-lisp.net/mailman/listinfo/tbnl-devel
_______________________________________________ tbnl-devel site list tbnl-devel@common-lisp.net http://common-lisp.net/mailman/listinfo/tbnl-devel
Hi Faré, On Fri, Aug 13, 2010 at 04:10, Faré <fahree@gmail.com> wrote:
Will do. I see no mention of this repository on the web page for the project: http://weitz.de/hunchentoot/ and it looked like that darcs repository was the closest thing to an upstream, but it might only have recorded public numbered releases.
The official upstream consists of the release tarballs. Development (currently) happens in the bknr Subversion repository, and patches against either of the two will (currently) be okay. As the darcs repository is not maintained by Edi or me, we can't tell whether it is up to date.
Is the bknr thirdparty svn now the official upstream for all ediware projects? Or at least the official "we're as close to Edi's disk as is otherwise published" mirror?
Edi and I commit changes to the bknr Subversion before releasing, but usually Edi makes the releases shortly after the commits.
i.e. is that also where I should be getting chunga, drakma, flexi-streams, cl-ppcre, cl-fad, cl-who, etc.?
The release tarballs found at weitz.de are the safest bet.
Are there also other software of which you are de facto maintainer, or should I fetch dependencies from their respective upstreams?
I am not aware of any abandoned projects that Edi's software depends on and that are de facto maintained by Edi, but he'll comment if I'm wrong. -Hans
This is all correct except that in the case of Hunchentoot I'm lacking somewhat behind in terms of the next release. Please send patches against the bknr repository and not against the latest release (which is from January). Thanks, Edi. On Fri, Aug 13, 2010 at 10:26 AM, Hans Hübner <hans.huebner@gmail.com> wrote:
Hi Faré,
On Fri, Aug 13, 2010 at 04:10, Faré <fahree@gmail.com> wrote:
Will do. I see no mention of this repository on the web page for the project: http://weitz.de/hunchentoot/ and it looked like that darcs repository was the closest thing to an upstream, but it might only have recorded public numbered releases.
The official upstream consists of the release tarballs. Development (currently) happens in the bknr Subversion repository, and patches against either of the two will (currently) be okay. As the darcs repository is not maintained by Edi or me, we can't tell whether it is up to date.
Is the bknr thirdparty svn now the official upstream for all ediware projects? Or at least the official "we're as close to Edi's disk as is otherwise published" mirror?
Edi and I commit changes to the bknr Subversion before releasing, but usually Edi makes the releases shortly after the commits.
i.e. is that also where I should be getting chunga, drakma, flexi-streams, cl-ppcre, cl-fad, cl-who, etc.?
The release tarballs found at weitz.de are the safest bet.
Are there also other software of which you are de facto maintainer, or should I fetch dependencies from their respective upstreams?
I am not aware of any abandoned projects that Edi's software depends on and that are de facto maintained by Edi, but he'll comment if I'm wrong.
-Hans
Dear Hunchentooters, here's my patch against the latest svn, de-xcvb-ified. That's what I'm running at ITA, it seems to be passing the QRes precheckin. Can you review, and apply if satisfactory? Suggested commit message (from Scott McKay's svn commit): Extend Hunchentoot's 'one-thread-per-connection-taskmaster' to support 'max-threads' semantics, i.e., don't create a new thread if we've max out. Add a 'pooled-thread-per-connection-taskmaster' that will eventually use a thread pool, if profiling indicates. Fix the 'handle-incoming-connection' to implement the new behavior. Add a commented-out implementation of 'accept-connections' that might give better performance. This needs to be discussed with the Hunchentoot maintainers. Address the review comments and discussions between Scott McKay and Hans Huebner, who is one of the H'toot maintainers. Also correctly issue HTTP 503 when the server runs out of threads. (work by Scott McKay) [ François-René ÐVB Rideau | Reflection&Cybernethics | http://fare.tunes.org ] Comparing oneself with Galileo or Einstein is certainly good for the ego — provided one refrains from going into too much detail. — John McCarthy
Thanks Faré, I have committed the patch, minus the whitespace-only changes. Everyone, please give it a spin and report any errors that you may find. -Hans 2010/8/21 Faré <fahree@gmail.com>:
Dear Hunchentooters,
here's my patch against the latest svn, de-xcvb-ified. That's what I'm running at ITA, it seems to be passing the QRes precheckin. Can you review, and apply if satisfactory?
Suggested commit message (from Scott McKay's svn commit):
Extend Hunchentoot's 'one-thread-per-connection-taskmaster' to support 'max-threads' semantics, i.e., don't create a new thread if we've max out.
Add a 'pooled-thread-per-connection-taskmaster' that will eventually use a thread pool, if profiling indicates. Fix the 'handle-incoming-connection' to implement the new behavior. Add a commented-out implementation of 'accept-connections' that might give better performance. This needs to be discussed with the Hunchentoot maintainers.
Address the review comments and discussions between Scott McKay and Hans Huebner, who is one of the H'toot maintainers. Also correctly issue HTTP 503 when the server runs out of threads.
(work by Scott McKay)
[ François-René ÐVB Rideau | Reflection&Cybernethics | http://fare.tunes.org ] Comparing oneself with Galileo or Einstein is certainly good for the ego — provided one refrains from going into too much detail. — John McCarthy
Faré, thanks for the patch which generally looks very good. However, I just had to revert it because it breaks the LispWorks implementation. I don't expect you to provide the new features for LW as well, but at least on LW the code should still compile and load in its old form - I'm relying on it for my daily work. Also, I don't see the need for factoring out the version number in a separate file. Thanks, Edi. 2010/8/21 Hans Hübner <hans.huebner@gmail.com>:
Thanks Faré,
I have committed the patch, minus the whitespace-only changes. Everyone, please give it a spin and report any errors that you may find.
-Hans
2010/8/21 Faré <fahree@gmail.com>:
Dear Hunchentooters,
here's my patch against the latest svn, de-xcvb-ified. That's what I'm running at ITA, it seems to be passing the QRes precheckin. Can you review, and apply if satisfactory?
Suggested commit message (from Scott McKay's svn commit):
Extend Hunchentoot's 'one-thread-per-connection-taskmaster' to support 'max-threads' semantics, i.e., don't create a new thread if we've max out.
Add a 'pooled-thread-per-connection-taskmaster' that will eventually use a thread pool, if profiling indicates. Fix the 'handle-incoming-connection' to implement the new behavior. Add a commented-out implementation of 'accept-connections' that might give better performance. This needs to be discussed with the Hunchentoot maintainers.
Address the review comments and discussions between Scott McKay and Hans Huebner, who is one of the H'toot maintainers. Also correctly issue HTTP 503 when the server runs out of threads.
(work by Scott McKay)
[ François-René ÐVB Rideau | Reflection&Cybernethics | http://fare.tunes.org ] Comparing oneself with Galileo or Einstein is certainly good for the ego — provided one refrains from going into too much detail. — John McCarthy
Dear Edi, On 22 August 2010 15:36, Edi Weitz <edi@agharta.de> wrote:
Faré, thanks for the patch which generally looks very good. However, I just had to revert it because it breaks the LispWorks implementation. I don't expect you to provide the new features for LW as well, but at least on LW the code should still compile and load in its old form - I'm relying on it for my daily work.
Also, I don't see the need for factoring out the version number in a separate file. Oh, that was an independent change I did to make h'toot more friendly to xcvb (that doesn't read the asd file thus will bork on the version) and the future more declarative asdf 3 (in which asd files would
Oops, I didn't realize the patch was not lispworks-friendly. I'll see if there's anything obvious I can do to make it compile as before on LispWorks. putatively not contain code?). I can separate it from the rest if you wish. [ François-René ÐVB Rideau | Reflection&Cybernethics | http://fare.tunes.org ] Mail addiction is a malediction.
On Mon, Aug 23, 2010 at 3:22 PM, Faré <fahree@gmail.com> wrote:
Oops, I didn't realize the patch was not lispworks-friendly. I'll see if there's anything obvious I can do to make it compile as before on LispWorks.
From a quick look, I'd say you just have to check that LispWorks doesn't see any of the symbols from Bordeaux Threads and usocket. If you end up with code where the new features only work on non-LispWorks compilers, that's not a problem. I'll take care of the rest then.
Oh, that was an independent change I did to make h'toot more friendly to xcvb (that doesn't read the asd file thus will bork on the version) and the future more declarative asdf 3 (in which asd files would putatively not contain code?). I can separate it from the rest if you wish.
Yes, please. Thanks, Edi.
On 23 August 2010 11:21, Edi Weitz <edi@agharta.de> wrote:
Oops, I didn't realize the patch was not lispworks-friendly. I'll see if there's anything obvious I can do to make it compile as before on LispWorks.
From a quick look, I'd say you just have to check that LispWorks doesn't see any of the symbols from Bordeaux Threads and usocket. If you end up with code where the new features only work on non-LispWorks compilers, that's not a problem. I'll take care of the rest then.
Hum. At some point we use make-condition-variable, which in bordeaux-threads is a non-trivial bit of code on lispworks. Is there a good reason to not use bordeaux-threads on lispworks? What is the right thing to do here?
Oh, that was an independent change I did to make h'toot more friendly to xcvb (that doesn't read the asd file thus will bork on the version) and the future more declarative asdf 3 (in which asd files would putatively not contain code?). I can separate it from the rest if you wish.
Yes, please.
Will do. [ François-René ÐVB Rideau | Reflection&Cybernethics | http://fare.tunes.org ] Wishing without work is like fishing without bait. — Frank Tyger
On Mon, Aug 23, 2010 at 7:48 PM, Faré <fahree@gmail.com> wrote:
Is there a good reason to not use bordeaux-threads on lispworks?
Yes, the LispWorks version of Hunchentoot (which is the original Hunchentoot which only ran on LW) doesn't use any of the compatibility libs like BT, usocket, and cl+ssl. I want it to stay that way - the less dependencies, the better.
What is the right thing to do here?
Can't you just comment out all the new stuff with #-:lispworks? As I said, I'll take care of the LW version. Cheers, Edi.
Dear Edi, Here's a revised patch against the current svn. It's xcvb and version changes reverted, otherwise what I'm running at ITA, and it seems to be passing the QRes precheckin. It looks like it passes the elementary test under Lispworks, but I don't personally have a working Lispworks setup, so I can't say (will it work on LW Personal?). Can you review and apply if satisfactory?
Yes, the LispWorks version of Hunchentoot (which is the original Hunchentoot which only ran on LW) doesn't use any of the compatibility libs like BT, usocket, and cl+ssl. I want it to stay that way - the less dependencies, the better.
Understood. I also saw that you defined trivial wrappers rather than imported LW symbols, and kept things that way.
Can't you just comment out all the new stuff with #-:lispworks? As I said, I'll take care of the LW version.
Scott did some work so it compiles under Lispworks, and though it's wholly untested, I fixed it rather than scrapped it. Please take it with a pinch of salt. Suggested commit message: Extend Hunchentoot's 'one-thread-per-connection-taskmaster' to support 'max-threads' semantics, i.e., don't create a new thread if we've max out. Add a 'pooled-thread-per-connection-taskmaster' that will eventually use a thread pool, if profiling indicates. Fix the 'handle-incoming-connection' to implement the new behavior. Add a commented-out implementation of 'accept-connections' that might give better performance. This needs to be discussed with the Hunchentoot maintainers. Address the review comments and discussions between Scott McKay and Hans Huebner. Also correctly issue HTTP 503 when the server runs out of threads. (Work by Scott McKay, merge by François-René Rideau) [ François-René ÐVB Rideau | Reflection&Cybernethics | http://fare.tunes.org ] Moving parts in rubbing contact require lubrication to avoid excessive wear. Honorifics and formal politeness provide lubrication where people rub together. Often the very young, the untraveled, the naïve, the unsophisticated deplore these formalities as "empty", "meaningless", or "dishonest", and scorn to use them. No matter how "pure" their motives, they thereby throw sand into machinery that does not work too well at best. — Robert Heinlein, "Time Enough For Love"
Thanks, that sounds good. Except that you forgot the attachment... :) On Wed, Aug 25, 2010 at 4:16 AM, Faré <fahree@gmail.com> wrote:
Dear Edi,
Here's a revised patch against the current svn. It's xcvb and version changes reverted, otherwise what I'm running at ITA, and it seems to be passing the QRes precheckin. It looks like it passes the elementary test under Lispworks, but I don't personally have a working Lispworks setup, so I can't say (will it work on LW Personal?).
Can you review and apply if satisfactory?
Yes, the LispWorks version of Hunchentoot (which is the original Hunchentoot which only ran on LW) doesn't use any of the compatibility libs like BT, usocket, and cl+ssl. I want it to stay that way - the less dependencies, the better.
Understood. I also saw that you defined trivial wrappers rather than imported LW symbols, and kept things that way.
Can't you just comment out all the new stuff with #-:lispworks? As I said, I'll take care of the LW version.
Scott did some work so it compiles under Lispworks, and though it's wholly untested, I fixed it rather than scrapped it. Please take it with a pinch of salt.
Suggested commit message:
Extend Hunchentoot's 'one-thread-per-connection-taskmaster' to support 'max-threads' semantics, i.e., don't create a new thread if we've max out.
Add a 'pooled-thread-per-connection-taskmaster' that will eventually use a thread pool, if profiling indicates. Fix the 'handle-incoming-connection' to implement the new behavior. Add a commented-out implementation of 'accept-connections' that might give better performance. This needs to be discussed with the Hunchentoot maintainers.
Address the review comments and discussions between Scott McKay and Hans Huebner. Also correctly issue HTTP 503 when the server runs out of threads.
(Work by Scott McKay, merge by François-René Rideau)
[ François-René ÐVB Rideau | Reflection&Cybernethics | http://fare.tunes.org ] Moving parts in rubbing contact require lubrication to avoid excessive wear. Honorifics and formal politeness provide lubrication where people rub together. Often the very young, the untraveled, the naïve, the unsophisticated deplore these formalities as "empty", "meaningless", or "dishonest", and scorn to use them. No matter how "pure" their motives, they thereby throw sand into machinery that does not work too well at best. — Robert Heinlein, "Time Enough For Love"
On 25 August 2010 02:49, Edi Weitz <edi@agharta.de> wrote:
Thanks, that sounds good. Except that you forgot the attachment... :)
Oops. Attached. [ François-René ÐVB Rideau | Reflection&Cybernethics | http://fare.tunes.org ] "We will never get the money out of politics until we get the politics out of money." — Alex Tabarrok
On Wed, Aug 25, 2010 at 4:16 AM, Faré <fahree@gmail.com> wrote:
Dear Edi,
Here's a revised patch against the current svn. It's xcvb and version changes reverted, otherwise what I'm running at ITA, and it seems to be passing the QRes precheckin. It looks like it passes the elementary test under Lispworks, but I don't personally have a working Lispworks setup, so I can't say (will it work on LW Personal?).
Can you review and apply if satisfactory?
Yes, the LispWorks version of Hunchentoot (which is the original Hunchentoot which only ran on LW) doesn't use any of the compatibility libs like BT, usocket, and cl+ssl. I want it to stay that way - the less dependencies, the better.
Understood. I also saw that you defined trivial wrappers rather than imported LW symbols, and kept things that way.
Can't you just comment out all the new stuff with #-:lispworks? As I said, I'll take care of the LW version.
Scott did some work so it compiles under Lispworks, and though it's wholly untested, I fixed it rather than scrapped it. Please take it with a pinch of salt.
Suggested commit message:
Extend Hunchentoot's 'one-thread-per-connection-taskmaster' to support 'max-threads' semantics, i.e., don't create a new thread if we've max out.
Add a 'pooled-thread-per-connection-taskmaster' that will eventually use a thread pool, if profiling indicates. Fix the 'handle-incoming-connection' to implement the new behavior. Add a commented-out implementation of 'accept-connections' that might give better performance. This needs to be discussed with the Hunchentoot maintainers.
Address the review comments and discussions between Scott McKay and Hans Huebner. Also correctly issue HTTP 503 when the server runs out of threads.
(Work by Scott McKay, merge by François-René Rideau)
[ François-René ÐVB Rideau | Reflection&Cybernethics | http://fare.tunes.org ] Moving parts in rubbing contact require lubrication to avoid excessive wear. Honorifics and formal politeness provide lubrication where people rub together. Often the very young, the untraveled, the naïve, the unsophisticated deplore these formalities as "empty", "meaningless", or "dishonest", and scorn to use them. No matter how "pure" their motives, they thereby throw sand into machinery that does not work too well at best. — Robert Heinlein, "Time Enough For Love"
Dear Edi, is there anything I can do to improve the patch that would help it make it to the official upstream hunchentoot? [ François-René ÐVB Rideau | Reflection&Cybernethics | http://fare.tunes.org ] Any time you're asking the user to make a choice they don't care about, you have failed the user — Jeff Atwood On 24 August 2010 22:16, Faré <fahree@gmail.com> wrote:
Dear Edi,
Here's a revised patch against the current svn. It's xcvb and version changes reverted, otherwise what I'm running at ITA, and it seems to be passing the QRes precheckin. It looks like it passes the elementary test under Lispworks, but I don't personally have a working Lispworks setup, so I can't say (will it work on LW Personal?).
Can you review and apply if satisfactory?
Yes, the LispWorks version of Hunchentoot (which is the original Hunchentoot which only ran on LW) doesn't use any of the compatibility libs like BT, usocket, and cl+ssl. I want it to stay that way - the less dependencies, the better.
Understood. I also saw that you defined trivial wrappers rather than imported LW symbols, and kept things that way.
Can't you just comment out all the new stuff with #-:lispworks? As I said, I'll take care of the LW version.
Scott did some work so it compiles under Lispworks, and though it's wholly untested, I fixed it rather than scrapped it. Please take it with a pinch of salt.
Suggested commit message:
Extend Hunchentoot's 'one-thread-per-connection-taskmaster' to support 'max-threads' semantics, i.e., don't create a new thread if we've max out.
Add a 'pooled-thread-per-connection-taskmaster' that will eventually use a thread pool, if profiling indicates. Fix the 'handle-incoming-connection' to implement the new behavior. Add a commented-out implementation of 'accept-connections' that might give better performance. This needs to be discussed with the Hunchentoot maintainers.
Address the review comments and discussions between Scott McKay and Hans Huebner. Also correctly issue HTTP 503 when the server runs out of threads.
(Work by Scott McKay, merge by François-René Rideau)
[ François-René ÐVB Rideau | Reflection&Cybernethics | http://fare.tunes.org ] Moving parts in rubbing contact require lubrication to avoid excessive wear. Honorifics and formal politeness provide lubrication where people rub together. Often the very young, the untraveled, the naïve, the unsophisticated deplore these formalities as "empty", "meaningless", or "dishonest", and scorn to use them. No matter how "pure" their motives, they thereby throw sand into machinery that does not work too well at best. — Robert Heinlein, "Time Enough For Love"
Buy me some time... :) Nah, not really. Sorry for the delay. I've been busy recently and just returned from a trip to New York. I'll try to squeeze this in in the next days. On Mon, Sep 13, 2010 at 1:39 AM, Faré <fahree@gmail.com> wrote:
Dear Edi,
is there anything I can do to improve the patch that would help it make it to the official upstream hunchentoot?
[ François-René ÐVB Rideau | Reflection&Cybernethics | http://fare.tunes.org ] Any time you're asking the user to make a choice they don't care about, you have failed the user — Jeff Atwood
On 24 August 2010 22:16, Faré <fahree@gmail.com> wrote:
Dear Edi,
Here's a revised patch against the current svn. It's xcvb and version changes reverted, otherwise what I'm running at ITA, and it seems to be passing the QRes precheckin. It looks like it passes the elementary test under Lispworks, but I don't personally have a working Lispworks setup, so I can't say (will it work on LW Personal?).
Can you review and apply if satisfactory?
Yes, the LispWorks version of Hunchentoot (which is the original Hunchentoot which only ran on LW) doesn't use any of the compatibility libs like BT, usocket, and cl+ssl. I want it to stay that way - the less dependencies, the better.
Understood. I also saw that you defined trivial wrappers rather than imported LW symbols, and kept things that way.
Can't you just comment out all the new stuff with #-:lispworks? As I said, I'll take care of the LW version.
Scott did some work so it compiles under Lispworks, and though it's wholly untested, I fixed it rather than scrapped it. Please take it with a pinch of salt.
Suggested commit message:
Extend Hunchentoot's 'one-thread-per-connection-taskmaster' to support 'max-threads' semantics, i.e., don't create a new thread if we've max out.
Add a 'pooled-thread-per-connection-taskmaster' that will eventually use a thread pool, if profiling indicates. Fix the 'handle-incoming-connection' to implement the new behavior. Add a commented-out implementation of 'accept-connections' that might give better performance. This needs to be discussed with the Hunchentoot maintainers.
Address the review comments and discussions between Scott McKay and Hans Huebner. Also correctly issue HTTP 503 when the server runs out of threads.
(Work by Scott McKay, merge by François-René Rideau)
[ François-René ÐVB Rideau | Reflection&Cybernethics | http://fare.tunes.org ] Moving parts in rubbing contact require lubrication to avoid excessive wear. Honorifics and formal politeness provide lubrication where people rub together. Often the very young, the untraveled, the naïve, the unsophisticated deplore these formalities as "empty", "meaningless", or "dishonest", and scorn to use them. No matter how "pure" their motives, they thereby throw sand into machinery that does not work too well at best. — Robert Heinlein, "Time Enough For Love"
Hans Hübner wrote:
I'd say that the idea of a thread pool can be abandoned if it is not useful, and it seems that it would not be useful.
And don't forget to put in comments or other documentation explaining why this decision was made, so that future developers (a) won't spend time putting it in (or asking why it's not in), and (b) will understand why that decision was made, in case things change (e.g. thread creation turns out to be expensive under some new, unforseen circumstance). -- Dan
participants (6)
-
Daniel Weinreb -
Edi Weitz -
Faré -
Gordon Sims -
Hans Hübner -
Scott McKay