ObservationListener.onError is no longer called on 5.00 responses

[niklas-sparfeld-gcx]

Version(s)

v2.0.0-M15

Which components

Leshan Server

Tested With

Our Unit Test suite using Leshan Client

What happened

With Leshan 2.0.0-M14 when a client sends an 5.00 observe response (or some other non-success response), then on the server side ObservationListener.onError is called.

With Leshan 2.0.0-M15 ObservationListener.onResponse is called instead. From what I can tell, this commit changed the behavior:

https://github.com/eclipse-leshan/leshan/commit/5bf2cd71dcd599b697c6a479e6453b02a656759f#diff-c6d441c87c91181049a57be76c639771aa4e5f53ba882a2bdf518b67c54dc8abL89-L94

Now, I believe the new behavior is more correct, maybe? If it is, then this is obviously not really a bug, but it is a breaking change that could be mentioned in the M15 release notes.

The thing is: Before M15 our onResponse handlers could assume that the response is valid and not an error, now everybody has add an additional check for response.isSuccess don't they?

Thanks!

How to reproduce

Here's a simple InstanceEnabler that we use:

         class BuggyEnabler : SimpleInstanceEnabler(0) {
            private val first = AtomicBoolean(true)

            override fun read(
                server: LwM2mServer?,
                resourceid: Int,
            ): ReadResponse? {
                return if (first.compareAndSet(true, false)) {
                    super.read(server, resourceid)
                } else {
                    ReadResponse.internalServerError("")
                }
            }
        }

Relevant Output

No response

[sbernard31]

@JaroslawLegierski, @mgdlkundera, @slaft maybe you should be aware of that.

@niklas-sparfeld-gcx thx a lot for taking time to reporting that. :pray:

The change was made on purpose. The idea from the beginning was :

• onResponse returns the response/notification from the device.
  • onError is if something bad happens (exception is raised : e.g. unable to decode payload)

(maybe I should improve the javadoc :thinking:)

Now, I believe the new behavior is more correct,

Good you think it's a better/good behavior.

it is a breaking change that could be mentioned in the M15 release notes.

I will add it but this is more a bug fix than an API change. I mean the previous behavior was not expected. (probably a regression when we add Transport Layer Abstraction)

Our Unit Test suite using Leshan Client

Do you want to be pinged just before our milestones release, so you can tests the content of the release just before we release code. So if you find issue there is chance we fix it before the release ?

[niklas-sparfeld-gcx]

👍 to everything and thanks for your quick response.

I already adjusted our test suite. Instead of sending an ReadResponse.internalServerError("") in order to trigger our onError code, I now implement a buggy LwM2MEncoder and send an invalid payload.

        val lwm2mClient =
            clientBuilder
                .createLeshanClientBuilder(/*...*/)
                .setEncoder(
                    object : DefaultLwM2mEncoder() {
                        private val first = AtomicBoolean(true)

                        override fun encode(
                            node: LwM2mNode,
                            format: ContentFormat,
                            path: LwM2mPath,
                            model: LwM2mModel,
                        ): ByteArray =
                            if (path.resourceId != 13) {
                                super.encode(node, format, path, model)
                            } else if (first.compareAndSet(true, false)) {
                                super.encode(node, format, path, model)
                            } else {
                                // this is invalid data. The server will fail to decode it -> CodecException -> InvalidResponseException -> onError
                                byteArrayOf(1, 2, 3)
                            }
                    },
                ).build()

Do you want to be pinged just before our milestones release

That would be nice. Normally I do that periodically in any case, but I was on parental leave for a while and did not find the time to notice M15 was about to be released. So if it's not too much hassle for you, it would be nice :)

[sbernard31]

it is a breaking change that could be mentioned in the M15 release notes.

Done : https://github.com/eclipse-leshan/leshan/releases/tag/leshan-2.0.0-M15

So if it's not too much hassle for you, it would be nice :)

Great ! I will do that.

I keep the issue open to not forget to improve javadoc of ObjectListener

[slaft]

@JaroslawLegierski, @mgdlkundera, @slaft maybe you should be aware of that.

Thanks a lot @sbernard31 for letting us know! Actually, I was not aware of this particular behavior for notifications prior to M15 (I expected onResponse to be called in this case).

[sbernard31]

I keep the issue open to not forget to improve javadoc of ObjectListener

I read again the javadoc and finally I think it is OK, I don't see anything to add : https://github.com/eclipse-leshan/leshan/blob/272bed38393e18b814f4985d4f51d030ac501c38/leshan-lwm2m-server/src/main/java/org/eclipse/leshan/server/observation/ObservationListener.java#L48-L79

So I close the issue.

If you think we can do better let me know :wink: