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

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

Closed
basimons opened this issue Jun 20, 2024 · 14 comments

Comments

@basimons
Copy link

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
Copy link
Contributor

boaks commented Jun 20, 2024

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
Copy link
Contributor

boaks commented Jun 20, 2024

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
Copy link
Author

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
Copy link
Contributor

boaks commented Jun 20, 2024

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 added a commit to boaks/californium that referenced this issue Jun 20, 2024
is provided as well.

Fix issue eclipse-californium#2252

Signed-off-by: Achim Kraus <achim.kraus@cloudcoap.net>
@boaks
Copy link
Contributor

boaks commented Jun 20, 2024

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

@basimons
Copy link
Author

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
Copy link
Contributor

boaks commented Jun 24, 2024

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

boaks added a commit that referenced this issue Jun 28, 2024
is provided as well.

Fix issue #2252

Signed-off-by: Achim Kraus <achim.kraus@cloudcoap.net>
@boaks
Copy link
Contributor

boaks commented Jun 28, 2024

@basimons

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

@basimons
Copy link
Author

basimons commented Jun 28, 2024 via email

@basimons
Copy link
Author

basimons commented Jul 2, 2024

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 added a commit that referenced this issue Jul 3, 2024
Fixes issue #2252.

Signed-off-by: Achim Kraus <achim.kraus@cloudcoap.net>
@boaks
Copy link
Contributor

boaks commented Jul 3, 2024

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
Copy link
Author

basimons commented Jul 3, 2024 via email

@basimons
Copy link
Author

basimons commented Jul 4, 2024

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

boaks added a commit that referenced this issue Jul 4, 2024
Add test and examples without payload.

Fixes issue #2252.

Signed-off-by: Achim Kraus <achim.kraus@cloudcoap.net>
boaks added a commit that referenced this issue Jul 4, 2024
Add test and examples without payload.

Fixes issue #2252.

Signed-off-by: Achim Kraus <achim.kraus@cloudcoap.net>
boaks added a commit that referenced this issue Jul 4, 2024
Add test and examples without payload.

Fixes issue #2252.

Signed-off-by: Achim Kraus <achim.kraus@cloudcoap.net>
@boaks
Copy link
Contributor

boaks commented Jul 10, 2024

Bugfix release 3.12.1 is available.

@boaks boaks closed this as completed Jul 10, 2024
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

2 participants