Consider returning a 500 instead of... nothing

[emberian]

$ curl -v localhost:1472

* Adding handle: conn: 0x24c8620
* Adding handle: send: 0
* Adding handle: recv: 0
* Curl_addHandleToPipeline: length: 1
* - Conn 0 (0x24c8620) send_pipe: 1, recv_pipe: 0
* About to connect() to localhost port 1472 (#0)
*   Trying ::1...
* Connection refused
*   Trying 127.0.0.1...
* Connected to localhost (127.0.0.1) port 1472 (#0)
> GET /user/cmr HTTP/1.1
> User-Agent: curl/7.32.0
> Host: localhost:1472
> Accept: */*
>
* Empty reply from server
* Connection #0 to host localhost left intact
curl: (52) Empty reply from server

But logged:

task '<unnamed>' failed at 'Error preparing statement:
ERROR: column "id" does not exist at position 8 in
SELECT id, username, password FROM User WHERE username = $1', lib.rs:565

I think the server should return a 500 here?

[chris-morgan]

I thought about it for a while, comparing what I have and considering the potential scenarios, and I've come to the conclusion that aborting the connection is probably the only reasonable behaviour here and at present the only feasible behaviour.

Some facts:

• The connection is aborted when the task which accepted it fails;
• A TCP connection can only be used in a single task (at present, at least);
• What I'm writing here is an HTTP library.

The design which would allow the sending of a 500 Internal Server Error response in this situation is:

• Come up with some way of using the TcpSocket across tasks (the most likely solution here would be replacing the TcpStream-using ResponseWriter with something using ports and channels, but I'm not enthusiastic about this; it's clearly something which needs consideration, however).
• Spawn a new task, using the future_result to detect whether that task fails; if it has failed and the headers have not been sent (ResponseWriter.headers_written as it is at present), then write a 500 Internal Server Error response.

Working across multiple tasks is clearly going to be something which must be supported somewhere along the line (I should consult with @brson about what the plan is with cross-task TCP sockets in the runtime—it could also affect the design for request pipelining), but at present I think we're best to avoid that complexity and inefficiency and wait.

Moreover, sending a generic 500 Internal Server Error response doesn't strike me as what the library should be doing; I'd think that to be the domain of a framework. (I haven't compared it with other HTTP libraries to see what they do, but that's my feeling on the matter.) Said framework could also do logging, etc. Constraining the scope of rust-http seems to me to be the sensible path here.

What thinkest thou in response?

[emberian]

That seems reasonable to me. The cross-task TcpStream is really something
that needs to be worked out; I've been bumping up against it quite a bit.

I just thought it was odd that as library user, task failure didn't trigger
a 500. Just needs documentation!

On Sun, Oct 6, 2013 at 12:27 AM, Chris Morgan [email protected]:

I thought about it for a while, comparing what I have and considering the
potential scenarios, and I've come to the conclusion that aborting the
connection is probably the only reasonable behaviour here and at present
the only feasible behaviour.

Some facts:

• The connection is aborted when the task which accepted it fails;
• A TCP connection can only be used in a single task (at present, at
  least);
• What I'm writing here is an HTTP library.

The design which would allow the sending of a 500 Internal Server Error
response in this situation is:

• Come up with some way of using the TcpSocket across tasks (the most
  likely solution here would be replacing the TcpStream-using
  ResponseWriter with something using ports and channels, but I'm not
  enthusiastic about this; it's clearly something which needs consideration,
  however).
• Spawn a new task, using the future_result to detect whether that
  task fails; if it has failed and the headers have not been sent (
  ResponseWriter.headers_written as it is at present), then write a 500
  Internal Server Error response.

Working across multiple tasks is clearly going to be something which must
be supported somewhere along the line (I should consult with @brsonhttps://github.com/brsonabout what the plan is with cross-task TCP sockets in the runtime—it could
also affect the design for request pipelining), but at present I think
we're best to avoid that complexity and inefficiency and wait.

Moreover, sending a generic 500 Internal Server Error response doesn't
strike me as what the library should be doing; I'd think that to be the
domain of a framework. (I haven't compared it with other HTTP libraries to
see what they do, but that's my feeling on the matter.) Said framework
could also do logging, etc. Constraining the scope of rust-http seems to me
to be the sensible path here.

What thinkest thou in response?

—
Reply to this email directly or view it on GitHubhttps://github.com//issues/22#issuecomment-25762161
.

[chris-morgan]

OK, docs it shall be.