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

FtRemote: The connection is reset on error instead of triggering an exception. #291

Closed
Bertram25 opened this issue May 15, 2015 · 5 comments

Comments

@Bertram25
Copy link
Contributor

As stated by @dmzaytsev in #253 , when consuming a bad request, FtRemote will break the connection instead of triggering an exception due to the illformed request, or when the multipart name isn't found.

Here is a debug test usable to reproduce the bug, based on org.takes.rq.RqMultipartTest.consumesHttpRequest: (Thanks again to @dmzaytsev for the lead about this.)

    public void consumesBadHttpRequest() throws IOException {
        final Take take = new Take() {
            @Override
            public Response act(final Request req) throws IOException {
                return new RsText(
                    new RqPrint(
                        new RqMultipart.Smart(
                            new RqMultipart.Base(req)
                        ).single("f-2") // Note that the requested name is wrong on purpose.
                    ).printBody()
                );
            }
        };
        new FtRemote(take).exec(
            // @checkstyle AnonInnerLengthCheck (50 lines)
            new FtRemote.Script() {
                @Override
                public void exec(final URI home) throws IOException {
                    new JdkRequest(home)
                        .method("POST")
                        .header(
                            "Content-Type",
                            "multipart/form-data; boundary=AaB0zz"
                        )
                        .body()
                        .set(
                            Joiner.on(RqMultipartTest.CRLF).join(
                                "--AaB0zz",
                                "Content-Disposition: form-data; name=\"f-1\"",
                                "",
                                "my picture",
                                "--AaB0zz--"
                            )
                        )
                        .back()
                        .fetch()
                        .as(RestResponse.class)
                        .assertStatus(HttpURLConnection.HTTP_OK)
                        .assertBody(Matchers.containsString("pic"));
                }
            }
        );
    }

Tested on OpenJDK7 (Linux 64bits)

@yegor256
Copy link
Owner

@Bertram25 can't we just reproduce the same problem with this:

final Take take = new Take() {
  @Override
  public Response act(final Request req) throws IOException {
    throw new HttpException(400);
  }
};

what do you think? for simplicity of the problem description

@Bertram25
Copy link
Contributor Author

@yegor256 I just tested by replacing the take variable declaration by yours, but the HTTPException then produced is correct.
Thus, IMO, the FtRemote instance does need more to create a socket exception.

I've tried to gather more details tonight toward a solution but the crash was too rare for that. I'll keep on doing this the next few days.

@yegor256
Copy link
Owner

@Bertram25 any update here?

@Bertram25
Copy link
Contributor Author

@yegor256 Sorry for the slow answer!

The error is gone thanks to newer changes (at least for me) and I wanted to bisect to make sure.

For info, the bug was visible at this commit: dde36ea
And is fixed for me at this commit: 4fd2637
using the both the test given in description and re-enabling the test in #253

Thus, to me, both this issue and #253 seem to be fixed by 4fd2637
(N.B.: To actually fix #253 , we still have to re-enabled the test, though.)

I also tested the test given in description, the test in #253 with latest changes for several minutes to make sure and everything is working for me so far.

If you agree, I can close this issue.

@yegor256
Copy link
Owner

yegor256 commented Jun 1, 2015

@Bertram25 thanks for the analysis! please close the issue

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

No branches or pull requests

2 participants