eclipse-californium / californium

CoAP/DTLS Java Implementation
https://www.eclipse.org/californium/
Other
730 stars 367 forks source link

Always a 5.02 response in ProxyHttpClientResource if http response contains no content-type header #2252

Closed basimons closed 4 months ago

basimons commented 5 months ago

Hello,

After updating from californium 2.7.0 -> 3.12, I ran into a small issue.

I have a resource like this:

@Slf4j
public class HttpClientResourceForwarder extends ProxyHttpClientResource {

    public HttpClientResourceForwarder (HttpProxyProperties props, MeterRegistry meterRegistry) {
        super(props.getCoapProxyVersionPath(), props.getCoapProxyVisible(), true,
            new Coap2ForwarderHttpTranslator(props, meterRegistry), props.getHttpScheme());
    }

    @Override
    public Resource getChild(String name) {
        // This ensures that all paths after the version path (/v0/**) end up at this Resource
        return this;
    }

    @Override
    public void handleRequest(final Exchange exchange) {
        log.debug("Received COAP proxy request: {}", exchange.getRequest());
        super.handleRequest(exchange);
    }
}

This worked fine in 2.7.0, but after upgrading I sometimes encounter an issue. This is caused when the server that the ProxyHttpClientResource does a request to, returns a response WITHOUT a content-type header, the response will always be a 5.02, with the message being "content type must not be null", which is set on org.eclipse.californium.proxy2.http.ContentTypedEntity#49.

IMO this is a bug, because getting a response without a Content-Type header is fine as long as the Content-Length is also 0.

Kind regards,

Bram

boaks commented 5 months ago

I guess the culprit is

CrossProtocolTranslator.setCoapPayload() .

If the "empty http content" is an empty array, then the check must be

if (payload != null && payload.length > 0)

Not sure, are you able to test that, if it fixes your issue?

boaks commented 5 months ago

Maybe it's ContentTypedEntity, at least the error message indicates that.

Then a fix may be:

    public ContentTypedEntity(ContentType contentType, byte[] payload) {
        this.payload = payload != null && payload.length > 0 ? payload : null;
        if (contentType == null && this.payload != null) {
            throw new NullPointerException("content type must not be null, if payload is provided!");
        }
        this.contentType = contentType;
    }

(I'm currently too busy with other stuff. I'm not sure, if I find some time before July to really work on this. Afterward I will add some unit tests and try to reproduce and fix that bug.)

basimons commented 5 months ago

It most definitely is the ContentTypedEntity. I went through it with the debugger and got this stacktrace:

java.lang.NullPointerException: content type must not be null!
    at org.eclipse.californium.proxy2.http.ContentTypedEntity.<init>(ContentTypedEntity.java:49)
    at org.eclipse.californium.proxy2.http.ContentTypedEntityConsumer.generateContent(ContentTypedEntityConsumer.java:68)
    at org.eclipse.californium.proxy2.http.ContentTypedEntityConsumer.generateContent(ContentTypedEntityConsumer.java:32)
    at org.apache.hc.core5.http.nio.entity.AbstractBinAsyncEntityConsumer.completed(AbstractBinAsyncEntityConsumer.java:82)
    at org.apache.hc.core5.http.nio.entity.AbstractBinDataConsumer.streamEnd(AbstractBinDataConsumer.java:81)
    at org.apache.hc.core5.http.nio.support.BasicResponseConsumer.streamEnd(BasicResponseConsumer.java:120)
    at org.apache.hc.client5.http.impl.async.HttpAsyncMainClientExec$1.streamEnd(HttpAsyncMainClientExec.java:251)
    at org.apache.hc.core5.http.impl.nio.ClientHttp1StreamHandler.dataEnd(ClientHttp1StreamHandler.java:270)
    at org.apache.hc.core5.http.impl.nio.ClientHttp1StreamDuplexer.dataEnd(ClientHttp1StreamDuplexer.java:366)
    at org.apache.hc.core5.http.impl.nio.AbstractHttp1StreamDuplexer.onInput(AbstractHttp1StreamDuplexer.java:335)
    at org.apache.hc.core5.http.impl.nio.AbstractHttp1IOEventHandler.inputReady(AbstractHttp1IOEventHandler.java:64)
    at org.apache.hc.core5.http.impl.nio.ClientHttp1IOEventHandler.inputReady(ClientHttp1IOEventHandler.java:41)
    at org.apache.hc.core5.reactor.InternalDataChannel.onIOEvent(InternalDataChannel.java:142)
    at org.apache.hc.core5.reactor.InternalChannel.handleIOEvent(InternalChannel.java:51)
    at org.apache.hc.core5.reactor.SingleCoreIOReactor.processEvents(SingleCoreIOReactor.java:178)
    at org.apache.hc.core5.reactor.SingleCoreIOReactor.doExecute(SingleCoreIOReactor.java:127)
    at org.apache.hc.core5.reactor.AbstractSingleCoreIOReactor.execute(AbstractSingleCoreIOReactor.java:86)
    at org.apache.hc.core5.reactor.IOReactorWorker.run(IOReactorWorker.java:44)
    at java.base/java.lang.Thread.run(Thread.java:840)

I hope that is helpful for you.

I don't think such a simple null check is enough to fix it, as you're going to have to introduce extra null checks on other places, where you now don't check the content type for null. Although I don't think you're going to be able to prevent that.

Thanks for your quick response and take your time :)

Bram

boaks commented 5 months ago

It's "just tracking the usage" ;-).

For now I see two places, where the content type is used and need to be checked. One will not be "called", because the payload is null and the other pass it to the http headers and is easy to fix.

And one usage in AsyncEntityProducers, that may require also some changes for "empty payloads".

Edited: empty payload may be just an null as producer.

So, I think, it requires some tests with empty contents in several cases to see if it works.

boaks commented 5 months ago

Still just a "try & error". If you like, please test, if PR #2253 works for you.

basimons commented 5 months ago

Awesome thanks for your quick help.

I'm running it in maven currently, so would be a bit hard for me to checkout the pr-branch and build it from source. Looking at the code though I'm positive that it should fix it. If its in the -SNAPSHOT branch I'm happy to try it out and see if it works.

boaks commented 4 months ago

I switched the 3.13.0-SNAPSHOT build to use the branch "issue_2252".

boaks commented 4 months ago

@basimons

Any result with the PR on the SNAPSHOT from your side?

basimons commented 4 months ago

I'll check it out Tuesday, I don't have access to my laptop before that. I'll let you know then.On 28 Jun 2024 10:34, Achim Kraus @.***> wrote: @basimons Any result with the PR on the SNAPSHOT from your side?

—Reply to this email directly, view it on GitHub, or unsubscribe.You are receiving this because you were mentioned.Message ID: @.***>

basimons commented 4 months ago

I can confirm, I ran all my tests, some of which failed with version 3.12, but now they all pass in version 3.13-SNAPSHOT.

boaks commented 4 months ago

With some tests: It seems, that only the http client calls ContentTypedEntityConsumer.generateContent() even if no entity is received. The http server seems not to call that and so doesn't fail with empty content.

I prepared an alternative fix in PR #2257 without introducing an API change. It's included in the current SNAPSHOT. If possible, could you please test it again? If the fix works for you as well, I would schedule a bugfix release 3.12.1 for next Tuesday.

basimons commented 4 months ago

Sure, I'll test it again tomorrow!

basimons commented 4 months ago

I ran the tests again, and they still pass fine!

boaks commented 4 months ago

Bugfix release 3.12.1 is available.