Reading empty body src hangs server

[benmerckx]

The tests are coming along but I'm having trouble reading the body properly if it's empty. Any server based on tcpcontainer fails here, without any Success or Failure but simply hangs indefinitely. I solved this previously in monsoon in a very hacky way. But I think this should be solved properly :)

[benmerckx]

So I realize the above report wasn't that helpful. Here's an example:

import tink.http.containers.TcpContainer;
import tink.http.Response;
using tink.CoreApi;

class Main {

    public static function main() {
        var container: TcpContainer = new TcpContainer(8000);
        container.run(function(req) {
            return switch (req.body) {
                case Plain(src):
                    return src.all().map(function (body)
                        return switch body {
                            case Success(bytes):
                                ('Plain body: '+bytes.toString(): OutgoingResponse);
                            case Failure(e):
                                ('Failed to parse body': OutgoingResponse);
                        }
                    );
                default:
                    Future.sync(('Parsed body': OutgoingResponse));
            }
        });
    }

}

This works when sending a request with a body attached (response is Plain body: ..body..). But a simple GET request without a body fails (no response, browser timeouts).

[back2dos]

Alright, thanks, I'll look into this.

[benmerckx]

I followed this path (we have a Source which has no bytes left so should result in EOF).

https://github.com/haxetink/tink_io/blob/master/src/tink/io/Buffer.hx#L214

var transfered = source.readBytes(bytes, end, toRead);

Input.readBytes doesn't throw EOF, but simply results in 0 bytes read.
This causes suspend to be called on StdSource:

https://github.com/haxetink/tink_io/blob/master/src/tink/io/Pipe.hx#L71

  function read()
    source.read(buffer).handle(function (o) switch o {
      case Success(_.isEof => true):
        source.close();
        buffer.seal();
        flush();
      case Success(v):
        if (v == 0 && buffer.available == 0)
          suspend(); // Here
        else
          flush();
      case Failure(e):
        terminate(SourceFailed(e));
    });

That ends up in a new read call because there's nothing in suspended:

https://github.com/haxetink/tink_io/blob/master/src/tink/io/Pipe.hx#L47

  function suspend() {
    if (this.bufferWidth > 0) 
      switch suspended.pop() {
        case null: read(); // Here
        case next:
          releaseBuffer();
          suspended.add(this);
          next.resume();
      }
    else read();
  }

So essentially this causes an infinite loop between read and suspend.

I'm unsure what suspended is meant for, so I don't really know how to fix this.

[back2dos]

Well, suspend puts a pending pipe into "standby" if you will. So that if you have many pipes the ones that have no data passing through, they don't compete with those that are busy.

Does the problem disappear if you always just read in suspend?

[benmerckx]

That puts me in the same position. Because read always ends up calling supend after if (v == 0 && buffer.available == 0) so we're looping again.

What fixes things for me is adding else return Progress.EOF; after this condition:
https://github.com/haxetink/tink_io/blob/master/src/tink/io/Buffer.hx#L219
But I know (think) that's not how it's supposed to work :)

[back2dos]

Ok, I have checked this, which was the most intelligible spec I could find: http://httpwg.org/specs/rfc7230.html#message.body

My understanding is that if there's neither Content-Length nor Transfer-Encoding, there's simply no body. And if the client doesn't close its end of the socket, we're out of luck. So in those cases I guess we could just use an empty source. Can you confirm?

[back2dos]

On second thought if no such thing is specified for POST, PUT or PATCH we might still try reading for "a while" just so that non-conforming clients get through. Or maybe all this should be a middleware anyway ...

[benmerckx]

I had to look up a lot of things :)
Seems you're right about content-length and transfer-encoding. In any case I think some timeouts should be set in place somewhere to prevent these loops in other scenarios.

What I wonder though is, when I tried running it through a streamparser I got -1. Does that signify the closing of the client's end or just that there's nothing to read at that time (but there could be data later on)?

[benmerckx]

After reading this I think I have most of it wrong :)

Is the problem the server waiting for a body, while the client is already waiting for a response? I've seen the curlclient succeed so I wonder why it's different there. I also still don't get when EOF can be expected, is that only used for closed connections?

[back2dos]

If curl shuts down its end of the socket then that would cause an EOF on the server. There are some chances that the TcpClient will work for the same reason.

Other clients (e.g. NodeClient) will leave the connection open for reuse. In that case the server receives no more data on the inbound stream but neither does it get an EOF. So yeah, the server must basically know in that case that no data is to be expected.

If you want to move forward with the tests, I suggest not to read the body for GET for now. Dealing with this properly entails implementing chunked encoding and using it when no content-length is specified (unless the underlying implementation takes care of it anyway).

[benmerckx]

I got it now, thanks! :)