fixes #1227 (https://github.com/yegor256/takes/issues/1227)

[EugenDueck]

When socket.getInputStream().available() returns 0, parsing of the request headers will be aborted. I guess the assumption was that available is 0 only in case of "end of stream", which is not true:

public int available()
Returns an estimate of the number of bytes that can be read (or skipped over) from this input stream without blocking, which may be 0, or 0 when end of stream is detected.

I think the appropriate thing to do is to get rid of the code that checks available() altogether, which is what this PR does. In order to reliably determine whether we have reached the end of the stream, we need to read(). No way around it.

[yegor256]

@EugenDueck I can't understand why github actions jobs are stuck. Can you please try to push something small into the branch, to restart them?

[yegor256]

@EugenDueck it seems that your test is stuck. Did you run the build locally? It passes?

[EugenDueck]

@yegor256 My bad! I hadn't run the other tests after fixing the issue.
So BkBasicTest.handlesTwoRequestInOneConnection() never finishes. I probably won't have the time to dig into it this weekend.
However, I noticed that the Content-Length of the first request in BkBasicTest.handlesTwoRequestInOneConnection() does not take into account the "\r\n" that Joined() adds after the body, so the Content-Length must be 13, rather than 11 (which makes me wonder why the test passed in the past):

                socket.getOutputStream().write(
                    new Joined(
                        BkBasicTest.CRLF,
                        BkBasicTest.POST,
                        BkBasicTest.HOST,
                        "Content-Length: 11",
                        "",
                        "Hello First",
                        BkBasicTest.POST,
                        BkBasicTest.HOST,
                        "Content-Length: 12",
                        "",
                        "Hello Second"
                    ).asString().getBytes()
                );

This still does not make the test finish. I will dig deeper when I find the time, hopefully next week.

[EugenDueck]

@yegor256

There was a deadlock in the test (the test waited for the server to close the socket, while the server waited for the client to close the socket). The commit I just pushed is supposed to fix that. With that, the tests all pass locally. As to the failing CI checks, I'm not sure how to read them. Looking at a couple of recent commits, most of them lead to failures and cancellations of one way or another.

However, looking into this issue lead me to discover a couple of issues that I would like to discuss, and therefore would like you to not merge this PR in its current form.

1. Supporting HTTP persistent connections #649 introduced the use of InputStream.available() into RqLive.parse(), presumably as a means of checking whether the end of the stream has been reached (i.e. the other side has closed its side of the connection). Unfortunately, InputStream.available() can't be relied upon for that - see the quote in my first message of this PR as to why. So what can happen, and what actually does happen in Invalid HTTP method: X-Takes-LocalAddress #1227, hence this PR, is that RqLive.parse() stops parsing the request, because available() returns 0. So this needs to be fixed, even though up to now, we have only seen the bug manifest on Windows.

2. Also in Supporting HTTP persistent connections #649, InputStream.available() was added to BkBasic.accept(), again, in order to check whether there is more data to come or not. This is after a request has been finished, waiting for the next request. So this means that Takes keeps the connection open only as long as the client can keep up "http pipelining" requests into the connection fast enough. The moment the client stops for just a tiny bit, Takes will disconnect. Which all but defeats the purpose of persistent connections. Http server implementations generally have a keep-alive timeout for persistent connections that defaults to anywhere between a couple of seconds to about 1 minute. Using .available(), the timeout is effectively "0". The tcpdump in the following screenshot, taken while doing

(echo -ne "GET /a HTTP/1.1\r\nHost: localhost:8100\r\n\r\n" ; cat) | nc localhost 8100

shows the immediate closure of the connection by Takes:

3. While looking into these issues, and wondering why BkBasicTest.handlesTwoRequestInOneConnection() succeeded even though the content length was wrong, I discovered \r and \n preceding the HTTP method in a HTTP request trip up the parser #1295

I don't feel like it is a good idea to fix 1., without also addressing 2. I would like to have a reliable "persistent connection" implementation first, with tests that show that it works, before fixing this bug, while making sure that persistent connections still work.

Actually, my preferred approach would be to proceed in this order:

1. remove support for persistent connections altogether (effectively rolling back Supporting HTTP persistent connections #649)
2. be happy (it will automatically fix this bug, but I would want to add a regression test to make sure it does and stays that way)
3. create a fresh "persistent connections" feature request for anyone willing to take up

Why do I suggest removing the persistent connections feature? Because that feature is barely working, as the tcpdump shows. And fixing the current implementation is not a trivial matter, in particular due to the need to implement a keep-alive timeout to prevent indefinite blocking of the connection and thread, which means coordinating with at least one more thread - unless of course we go NIO, which however would amount to a complete rewrite of the whole framework.

So we would just remove something that cannot be used in its current form, cannot be fixed easily, but introduces problems.

What do you think?