Skip to content

Commit

Permalink
Don't wrap the handler body in with-mapped-conditions
Browse files Browse the repository at this point in the history
with-mapped-conditions behaves differently on different platforms. In
particular on SBCL it takes an arbitrary signal, and renames it to
something like usocket:unknown-signal. In itself this is usually not
bad, because usually there's nothing wrapping
process-connection/process-request.

But I'm trying to implement
#185, basically make
Lispworks use the same code path as all other Lisps. BUT, this hits a
small issue, because usocket:with-mapped-conditions does some worse
things on Lispworks. It takes that arbitrary condition and converts
that into an error, which obviously brings down the request in a bad
way when it wasn't even an error to begin with. (In my case, the
signal was showing up when asdf was signalling system-out-of-date,
which shouldn't be an error)

I don't know why this with-mapped-condition is here. I can't tell much
from the git history. It feels like it shouldn't be here at all. But I
decided to take a conservative approach and move the
with-mapped-conditions to just other hunchentoot code, and just made
sure it's not wrapping handler bodies.

This should be the only issue blocking #185, if this is accepted I'll
be testing out my change for #185 for a while and then send out a PR.
  • Loading branch information
tdrhq committed Sep 28, 2020
1 parent d684a90 commit 42d9da3
Show file tree
Hide file tree
Showing 2 changed files with 32 additions and 32 deletions.
3 changes: 1 addition & 2 deletions acceptor.lisp
Original file line number Diff line number Diff line change
Expand Up @@ -365,8 +365,7 @@ This is supposed to force a check of ACCEPTOR-SHUTDOWN-P."
;; this around method is used for error handling
;; note that this method also binds *ACCEPTOR*
(with-conditions-caught-and-logged ()
(with-mapped-conditions ()
(call-next-method))))
(call-next-method)))

(defun do-with-acceptor-request-count-incremented (*acceptor* function)
(with-lock-held ((acceptor-shutdown-lock *acceptor*))
Expand Down
61 changes: 31 additions & 30 deletions request.lisp
Original file line number Diff line number Diff line change
Expand Up @@ -223,35 +223,36 @@ slot values are computed in this :AFTER method."
*headers-sent*
(*request* request))
(unwind-protect
(with-mapped-conditions ()
(labels
((report-error-to-client (error &optional backtrace)
(when *log-lisp-errors-p*
(log-message* *lisp-errors-log-level* "~A~@[~%~A~]" error (when *log-lisp-backtraces-p*
backtrace)))
(labels
((report-error-to-client (error &optional backtrace)
(when *log-lisp-errors-p*
(log-message* *lisp-errors-log-level* "~A~@[~%~A~]" error (when *log-lisp-backtraces-p*
backtrace)))
(with-mapped-conditions ()
(start-output +http-internal-server-error+
(acceptor-status-message *acceptor*
+http-internal-server-error+
:error (princ-to-string error)
:backtrace (princ-to-string backtrace)))))
(multiple-value-bind (contents error backtrace)
;; skip dispatch if bad request
(when (eql (return-code *reply*) +http-ok+)
(catch 'handler-done
(handle-request *acceptor* *request*)))
(when error
;; error occurred in request handler
(report-error-to-client error backtrace))
(unless *headers-sent*
(handler-case
(with-debugger
(start-output (return-code *reply*)
(or contents
(acceptor-status-message *acceptor*
(return-code *reply*)))))
(error (e)
;; error occurred while writing to the client. attempt to report.
(report-error-to-client e)))))))
:backtrace (princ-to-string backtrace))))))
(multiple-value-bind (contents error backtrace)
;; skip dispatch if bad request
(when (eql (return-code *reply*) +http-ok+)
(catch 'handler-done
(handle-request *acceptor* *request*)))
(when error
;; error occurred in request handler
(report-error-to-client error backtrace))
(unless *headers-sent*
(with-mapped-conditions ()
(handler-case
(with-debugger
(start-output (return-code *reply*)
(or contents
(acceptor-status-message *acceptor*
(return-code *reply*)))))
(error (e)
;; error occurred while writing to the client. attempt to report.
(report-error-to-client e)))))))
(dolist (path *tmp-files*)
(when (and (pathnamep path) (probe-file path))
;; the handler may have chosen to (re)move the uploaded
Expand All @@ -271,8 +272,8 @@ alist or NIL if there was no data or the data could not be parsed."
(let* ((content-length (header-in :content-length request))
(content-stream (make-flexi-stream (content-stream request)
:external-format +latin-1+
:bound (if content-length
(parse-integer content-length
:bound (if content-length
(parse-integer content-length
:junk-allowed t)))))
(prog1
(parse-rfc2388-form-data content-stream (header-in :content-type request) external-format)
Expand Down Expand Up @@ -326,8 +327,8 @@ unknown character set ~A in request content type."
external-format))
((and (string-equal type "multipart")
(string-equal subtype "form-data")
(if content-length
(plusp (parse-integer content-length
(if content-length
(plusp (parse-integer content-length
:junk-allowed t))
t))
(prog1 (parse-multipart-form-data request external-format)
Expand All @@ -350,7 +351,7 @@ during the request."
(setf (slot-value request 'get-parameters)
(form-url-encoded-list-to-alist (split "&" (query-string request)) external-format))
(values))

(defun script-name* (&optional (request *request*))
"Returns the file name of the REQUEST object REQUEST. That's the
requested URI without the query string \(i.e the GET parameters)."
Expand Down

0 comments on commit 42d9da3

Please sign in to comment.