Don't wrap the handler body in with-mapped-conditions #186
Conversation
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 edicl#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 edicl#185, if this is accepted I'll be testing out my change for edicl#185 for a while and then send out a PR.
| (handle-request *acceptor* *request*))) | ||
| (when error | ||
| ;; error occurred in request handler | ||
| (with-mapped-conditions () |
There was a problem hiding this comment.
I brought back the with-mapped-conditions here...
| (with-mapped-conditions () | ||
| (report-error-to-client error backtrace))) | ||
| (unless *headers-sent* | ||
| (with-mapped-conditions () |
|
Shouldn't usocket:with-mapped-conditions be fixed if it isn't behaving consistently? |
Yes, but the problem here is the with-mapped-conditions ideally shouldn't be used from hunchentoot at all. My understanding of with-mapped-conditions is that it converts implementation specific conditions into usocket external conditions. (It does this buggily though, and in fact its tests that check if it's throwing the right conditions are failing, at least on SBCL). So essentially, this should be internal to usocket, but it was exposed at some point (I think for hunchentoot). So maybe there's some reason some hunchentoot code needs to be wrapped with this (perhaps hunchentoot is using/used some other internal usocket function that needs to be wrapped with this). But, it shouldn't wrap around code that isn't hunchentoot internal code (i.e. handler bodies). |
|
Ok, gotta remove with-mapped-conditions altogether then. |
|
@stassats Ideally, yes. Would you prefer I remove it completely? We might discover why somebody put it in the first place, but that's okay. |
|
The original idea was to map the conditions thrown by the different implementations to a set of conditions that could be handled uniformly in usocket and usocket client code. |
|
I suppose usocket can't handle conditions coming from streams, so, no, completely removing it wouldn't work. |
|
So, it doesn't matter where with-mapped-condition is as long as it only handles socket errors. Usocket is the place to fix this. |
|
@stassats But the with-mapped-condition might also handle socket errors unrelated to the hunchentoot request (for instance if the handler body does some socket code, say for instance connecting to mysql or something like that). If streams are the only issue, I can wrap a gray-stream over the stream returned by usocket, and do with-wrapped-conditions over those methods. (Ideally, usocket should've done this, but it doesn't look like it's doing it.) @hanshuebner Can you verify that this solution would work? Or are there are other situations that I'm not thinking of? |
But it's still a socket condition, and it'll only apply outside of the handler.
No, gray streams is a bad idea. |
Elaborate? Would this be a performance concern? Or something else? |
It's everything. It's not what usocket returns, it's not what underlying interface returns, nothing works on it, it's slow. |
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.