mmazi / rescu

A lightweight json Rest client utility for JAX-RS
MIT License
65 stars 60 forks source link

PUT, POST hangs untill timeout if content is empty #119

Open sergey-erkin opened 5 years ago

sergey-erkin commented 5 years ago

The problem My thread hangs when no body passed into PUT or POST params (I use url params only) But everything is OK if I change method to GET

Possible solution I suppose we need to do connection.setDoOutput() for PUT and POST methods regardless request content length: see HttpTemplate.java:172

    if (contentLength > 0 || method == POST || method == PUT || method == DELETE) {
        connection.setDoOutput(true);
        connection.setDoInput(true);
    }
    connection.setRequestProperty("Content-Length", Integer.toString(contentLength));

Also we need to open and flush output stream here HttpTemplate.java:114

    if (contentLength > 0  || method == POST || method == PUT || method == DELETE) {
        // Write the request body
        OutputStream out = connection.getOutputStream();
        out.write(requestBody.getBytes(CHARSET_UTF_8));
        out.flush();
    }

Two changes above help me

mmazi commented 5 years ago

Thanks for reporting. Can you paste your code? Especially the interface method(s) where the problem manifests. IIUC it should be something like this:

@POST
@Path("...")
public void doSomething(@QueryParam("...") String firstParam) throws IOException;

Plus the code where you create the client proxy and where you call the method.

If you can put together a test case, that would help a lot in having the issue resolved quickly.

Thanks!

sergey-erkin commented 5 years ago

Here the piece of code is

  1. Interface

    @PUT
    @Path("/order")
    @Produces(APPLICATION_JSON)
    PutOrderResponse putOrder(
            @QueryParam("type") String type,
            @QueryParam("account") String account,
            @QueryParam("class") String classCode,
            @QueryParam("security") String securityCode,
            @QueryParam("operation") String operationCode,
            @QueryParam("quantity") String quantity,
            @QueryParam("price") String price
    ) throws IOException;
  2. Create proxy

        ClientConfig rescuConfig = new ClientConfig();
        rescuConfig.setJacksonObjectMapperFactory(
                new DefaultJacksonObjectMapperFactory() {
                    @Override
                    public void configureObjectMapper(ObjectMapper mapper) {
                        super.configureObjectMapper(mapper);
                        mapper.configure(WRAP_EXCEPTIONS, false);
                        mapper.configure(ACCEPT_FLOAT_AS_INT, false);
                        mapper.configure(FAIL_ON_INVALID_SUBTYPE, false);
                        mapper.configure(USE_BIG_DECIMAL_FOR_FLOATS, true);
                        mapper.configure(FAIL_ON_UNKNOWN_PROPERTIES, false);
                        mapper.configure(FAIL_ON_NULL_FOR_PRIMITIVES, true);
                        mapper.configure(ADJUST_DATES_TO_CONTEXT_TIME_ZONE, false);
                        mapper.configure(ACCEPT_EMPTY_STRING_AS_NULL_OBJECT, true);
                        mapper.configure(ACCEPT_EMPTY_ARRAY_AS_NULL_OBJECT, true);
                        mapper.configure(READ_DATE_TIMESTAMPS_AS_NANOSECONDS, false);
                    }
                }
        );
        restInterface = RestProxyFactory.createProxy(QUIKRestInterface.class, address, rescuConfig, new ConnectionLogger());
  3. I have no tests for it and do not plan them

mmazi commented 5 years ago

Thanks, I'll have a look when I find the time (probably next week).

Here's some info for reference about such requests: https://stackoverflow.com/questions/1233372/is-an-http-put-request-required-to-include-a-body

My take is that we shouldn't do PUT requests without a body (or without a Content-Length header), but I guess we should support empty bodies with PUT by sending Content-Length: 0.

sergey-erkin commented 5 years ago

Thank you! By the way when next release should come if you make a fix?

mmazi commented 5 years ago

I'm doing some research. The issue is not as trivial as it seemed at first so it might take a bit longer; any help appreciated.

It wasn't able to reproduce this with a test server; see the added test case in this branch: https://github.com/mmazi/rescu/tree/issue-119

It seems to me that your thread hangs because the server you connect to does not respond. (Note that the server in the test case responds normally.) Since I believe the HTTP request created by rescu in your case conforms to the HTTP specification, I'm not convinced rescu needs to be changed.

There seems to be an issue (I understand it is a security feature and not a bug) with HttpURLConnection that overwrites/strips the Content-Length header if no body was written: https://stackoverflow.com/questions/6056125/why-does-content-length-http-header-field-use-a-value-other-than-the-one-given-i

So perhaps setting the System property sun.net.http.allowRestrictedHeaders to true could be a solution for you, if this is possible.

I suspect your server fails to respond because the Content-Length header is not set. Can you confirm this? I.e. is it true that a request with Content-Length = 0 succeeds and an otherwise identical request with this header missing hangs?

A side note: an empty string (empty body) is not a valid json so @Consumes(APPLICATION_JSON) should not be used here.

These are some bits of information I've found. I hope this is helpful. I don't have much time for this issue currently; if you could research further yourself this would be great.

sergey-erkin commented 5 years ago

Hi mmazi! Thank a lot for your investigations and detailed answer!

  1. About point "because the server you connect to does not respond" No. The server responds well on PUT, GET, POST and DELETE requests if I send them from SoapUI. Also as I mentioned it is working good when my threads sends GET requests.

  2. About point "setting the System property sun.net.http.allowRestrictedHeaders to true" Yeah! Setting this property fixes the problem But I concern this solution is too "global" to me.

  3. About point "server fails to respond because the Content-Length header is not set" May be this is true, but unfortunatelly I can't check sended headers from server side. Do you have any idea how to check it from client side? By the way SoapUI sends Content-Length=0

  4. Yes, @Consumes(APPLICATION_JSON) is wrong here Thank you for your note

To recap For now I see good way to find solution: to provide Content-Length=0 Should I setup headers using rescu or rescu should do it itself?

mmazi commented 5 years ago

What you wrote seems to fit with my suspicion that your server does not respond to a PUT request with no Content-Length header set (that's what I meant).

When using rescu, you can see the request headers rescu sends by setting <logger name="si.mazi.rescu" level="trace"/> in logback.xml (see examples in rescu and XChange). You can't, however, see the response headers in the problematic case because (as it seems) the server does not seem to send them (it doesn't send any response).

Setting the Content-Length header explicitly on the client won't help due to the mentioned feature of HttpURLConnection.

At this point, I see a few possible next steps:

  1. Just use allowRestrictedHeaders=true, done
  2. Try sending a dummy non-empty body (e.g an empty json object "{}" or perhaps a space " ") and see what happens
  3. Your original description with changes in HttpTemplate that helped actually seems to indicate that it is possible to change rescu so that this will work. I am, however, reluctant to go in this direction because there is no good way to check that this won't break the many clients that use rescu (e.g. all exchange implementations in XChange). Also, I am not sure what would be the best way to do this (your suggestion with || method == POST || ... doesn't seem correct).