Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

TkSlf4j can fail for specific request #712

Closed
g4s8 opened this issue Sep 7, 2016 · 48 comments
Closed

TkSlf4j can fail for specific request #712

g4s8 opened this issue Sep 7, 2016 · 48 comments

Comments

@g4s8
Copy link
Contributor

g4s8 commented Sep 7, 2016

Hi.
If TkSlf4j wraps Take that returns empty response (without body),
and if POST request with some body has been sent through VerboseWire (from jcabi-http),
RqMethod.Base will fail with this exception:

java.io.IOException: Invalid HTTP method: X-Takes-LocalAddress:
    at org.takes.rq.RqMethod$Base.method(RqMethod.java:127)
    at org.takes.tk.TkSlf4j.act(TkSlf4j.java:105)
    at org.takes.http.BkBasic.print(BkBasic.java:108)
    at org.takes.http.BkBasic.accept(BkBasic.java:84)
    at org.takes.http.BkParallel$1$1.run(BkParallel.java:89)
    at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1142)
    at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:617)
    at java.lang.Thread.run(Thread.java:745)

Test for reproducing:

    @Test
    public void test() throws IOException {
        new FtRemote(
            new TkSlf4j(
                req -> new RsEmpty()
            )
        ).exec(
            home ->
                new JdkRequest(home)
                    .method("POST")
                    .body().set("test").back()
                    .through(VerboseWire.class)
                    .fetch()
                    .as(RestResponse.class)
                    .assertStatus(HttpURLConnection.HTTP_OK)
        );
    }

Environment:
java - 1.8.0_102-b14
takes - 1.1
jcabi-http - 1.16

http:

POST / HTTP/1.1
Cache-Control: no-cache
Pragma: no-cache
User-Agent: Java/1.8.0_102
Host: localhost:41997
Accept: text/html, image/gif, image/jpeg, *; q=.2, */*; q=.2
Connection: keep-alive
Content-type: application/x-www-form-urlencoded
Content-Length: 4

testHTTP/1.1 200 OK

HTTP/1.1 500 Internal Error
Content-Length: 538
Content-Type: text/plain

java.io.IOException: Invalid HTTP method: X-Takes-LocalAddress:
.at org.takes.rq.RqMethod$Base.method(RqMethod.java:127)
.at org.takes.tk.TkSlf4j.act(TkSlf4j.java:105)
.at org.takes.http.BkBasic.print(BkBasic.java:108)
.at org.takes.http.BkBasic.accept(BkBasic.java:84)
.at org.takes.http.BkParallel$1$1.run(BkParallel.java:89)
.at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1142)
.at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:617)
.at java.lang.Thread.run(Thread.java:745)
@davvd
Copy link

davvd commented Sep 10, 2016

@yegor256 please pay attention to this issue (par.21)

@yegor256 yegor256 added the bug label Nov 22, 2016
@yegor256
Copy link
Owner

@g4s8 definitely a bug, thanks for reporting

@davvd
Copy link

davvd commented Nov 22, 2016

@g4s8 attached this issue to milestone "1.0" (let me know if this is wrong)

@davvd davvd added this to the 1.0 milestone Nov 22, 2016
@davvd
Copy link

davvd commented Nov 22, 2016

@dalifreire please pick this up, and keep in mind these instructions. Any technical questions - ask right here

Task's budget is 30 mins (see this for explanation)

@dalifreire
Copy link
Contributor

@davvd Please, find someone else... I'll be away for the next weeks. Thanks.

@davvd
Copy link

davvd commented Nov 23, 2016

@davvd Please, find someone else... I'll be away for the next weeks. Thanks.

@dalifreire deducted 30 from your rating :(

@davvd
Copy link

davvd commented Nov 23, 2016

@davvd Please, find someone else... I'll be away for the next weeks. Thanks.

@dalifreire someone else will help in this task, no problem at all

@davvd davvd removed the @dalifreire label Nov 23, 2016
@davvd
Copy link

davvd commented Nov 23, 2016

@rui-castro This task is yours, please go ahead keeping in mind this. If any questions, don't hesitate to ask right here; This task's budget is 30 mins. This is exactly how much will be paid when the problem explained above is solved. See this for more information

@rui-castro
Copy link
Contributor

@davvd done in PR #716

The problem reported is caused by BkBasic not handling requests properly.

I'm not entirely confident that I diagnosed the problem correctly, because it seems very unlikely that this problem in BkBasic only comes up now. Anyway, this is what I've found...

The Take in the test req -> new RsEmpty() doesn't read the request body, so the body remains in the InputStream.

After the Take is finished, BkBasic checks if there is more content (supposedly a new request) in the InputStream. Since the body of the previous request is still in the InputStream, BkBasic constructs a new RqLive with that content and passes it to the Take.

The new request doesn't have a method or body and has only the headers X-Takes-... injected by BkBasic.

In the test example, since the TkSlf4j tries to log the request info method, url and headers, it fails with error Invalid HTTP method: X-Takes-LocalAddress:.

The proposed solution introduces a new Take (TkConsumeBody) that is always used to wrap the RqLive and that, after the inner Take executes, it consumes (skip) the remaining body data accordingly to Content-Length or Transfer-Encoding: chuncked.

@rui-castro
Copy link
Contributor

@davvd the PR #716 is ready for 6 days. Could you please do something to close the PR and this issue?

@davvd
Copy link

davvd commented Dec 8, 2016

@rui-castro you're working with this ticket for the last 15 days. If it is not closed within the next 48 hours, it will be re-assigned to someone else, see No Obligations principle. This article should help if you're stuck... -30 added to your rating, at the moment it is: -30

@rui-castro
Copy link
Contributor

@davvd That's not fair! The work is done and waiting for @yegor256 approval for 7 days.

@davvd
Copy link

davvd commented Dec 13, 2016

@rui-castro the task is overdue, and I have to re-assign it to someone else. Please, stop working with it immediately. In general, we're against overdue tasks, check this page; added -60 to your rating, now it is equal to -90

@davvd davvd removed the @rui-castro label Dec 13, 2016
@davvd
Copy link

davvd commented Dec 13, 2016

@prondzyn it's yours now, please proceed keeping in mind our principles. Feel free to ask any technical questions right here in the ticket; Total fixed cost of this task is 30 mins (see this for more info)

@prondzyn
Copy link
Contributor

@davvd I'm on it.

prondzyn added a commit to prondzyn/takes that referenced this issue Dec 14, 2016
@prondzyn
Copy link
Contributor

@davvd PR #717 is ready.

@davvd
Copy link

davvd commented Dec 14, 2016

@davvd PR #717 is ready.

@prondzyn thank you

@davvd
Copy link

davvd commented Jan 21, 2017

@davvd still need more time here. I'm waiting on @yegor256's in #717

@prondzyn sure, take your time, I know about this pause

@prondzyn
Copy link
Contributor

@davvd still need more time here. I'm waiting on @yegor256's in #717

@davvd
Copy link

davvd commented Jan 31, 2017

@davvd still need more time here. I'm waiting on @yegor256's in #717

@prondzyn sure, take your time, I know about this pause

@prondzyn
Copy link
Contributor

@davvd still need more time here. I'm waiting on @yegor256's in #717

@davvd
Copy link

davvd commented Feb 10, 2017

@davvd still need more time here. I'm waiting on @yegor256's in #717

@prondzyn take your time, I am aware that we're waiting here

prondzyn added a commit to prondzyn/takes that referenced this issue Feb 15, 2017
prondzyn added a commit to prondzyn/takes that referenced this issue Feb 15, 2017
prondzyn added a commit to prondzyn/takes that referenced this issue Feb 16, 2017
@prondzyn
Copy link
Contributor

@davvd still need more time here. I'm waiting on @yegor256's in #717

@davvd
Copy link

davvd commented Feb 20, 2017

@davvd still need more time here. I'm waiting on @yegor256's in #717

@prondzyn yeah, I know, take your time

@prondzyn
Copy link
Contributor

prondzyn commented Mar 2, 2017

@davvd still need more time here. I'm waiting on @yegor256's in #717

@davvd
Copy link

davvd commented Mar 2, 2017

@davvd still need more time here. I'm waiting on @yegor256's in #717

@prondzyn take your time, I am aware that we're waiting here

prondzyn added a commit to prondzyn/takes that referenced this issue Mar 9, 2017
@prondzyn
Copy link
Contributor

@davvd still need more time here. I'm waiting on @yegor256's in #717

@davvd
Copy link

davvd commented Mar 15, 2017

@davvd still need more time here. I'm waiting on @yegor256's in #717

@prondzyn sure, take your time, I know about this pause

@prondzyn
Copy link
Contributor

@davvd still need more time here. I'm waiting on @yegor256's in #717

@davvd
Copy link

davvd commented Mar 27, 2017

@davvd still need more time here. I'm waiting on @yegor256's in #717

@prondzyn sure, take your time, I know about this pause

@prondzyn
Copy link
Contributor

prondzyn commented Apr 2, 2017

@davvd still need more time here. I'm waiting on @yegor256's in #717

@davvd
Copy link

davvd commented Apr 2, 2017

@davvd still need more time here. I'm waiting on @yegor256's in #717

@prondzyn sure, take your time, I know about this pause

@prondzyn
Copy link
Contributor

@davvd still need more time here. I'm waiting on @yegor256's in #717

@davvd
Copy link

davvd commented Apr 12, 2017

@davvd still need more time here. I'm waiting on @yegor256's in #717

@prondzyn take your time, I am aware that we're waiting here

@prondzyn
Copy link
Contributor

prondzyn commented Jun 8, 2017

@g4s8 could you close this issue, please? I pushed test to reproduce the issue and additional task (#730) was automatically created to resolve the issue.

@g4s8
Copy link
Contributor Author

g4s8 commented Jun 8, 2017

@prondzyn ok, thanks

@g4s8 g4s8 closed this as completed Jun 8, 2017
@0crat
Copy link
Collaborator

0crat commented Jun 8, 2017

Oops! Job gh:yegor256/takes#712 was not in scope

@prondzyn
Copy link
Contributor

@davvd the task is closed.

@0pdd
Copy link
Collaborator

0pdd commented Nov 18, 2018

@g4s8 the puzzle #869 is still not solved; solved: #730, #731, #800.

@0pdd
Copy link
Collaborator

0pdd commented Nov 26, 2019

@g4s8 all 4 puzzles are solved here: #730, #731, #800, #869.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

8 participants