Better handle unexpected error in DefaultRegistrationEngine.

[moznion]

Hello,

If the procedure inside of ClientInitiatedBootstrapTask, UpdateRegistrationTask, and/or RegistrationTask raises an unexpected exception, that exception caught by a catch clause and it logs that, but that task never works after caught and unfortunately the process still alive.

For example, if registerWithRetry() that is in RegistrationTask#run() raises a RuntimeException, that task never scheduled anymore (i.e. that process does nothing).

This behavior makes it a little bit hard to implement a robust LwM2M client. I think it would be better to provide a way to exit when it falls into an unexpected situation.

What do you think about this?

[sbernard31]

Hi @moznion, thx for reporting this 🙏 and sorry for the delay.

I think you raise a good point. I looked at you PR #928 and you propose to add a callback.

Before to go in this way I would discuss a bit about this.

1. When this happens do we really want to stop the device or we want to reschedule a task ?

I suppose that if something unexpected happens this is maybe better to stop the client.

2. Is there something else to do than stopping the client ? in other word do we really need a callback ?

This behavior makes it a little bit hard to implement a robust LwM2M client. I think it would be better to provide a way to exit when it falls into an unexpected situation.

Should we directly stop the client instead of let user to do it ? (I mean stop the client in leshan library not only in leshan-client-demo)

3. if we need a callback maybe we can reuse LwM2mClientObserver ?

instead of creating a new one ?

4. Looking at PR #928, it seems you need to execute code in LwM2mObjectEnabler too ?

Maybe we can rather add a new method in this interface to handle this ?

[moznion]

Thank you for your response, @sbernard31!

1. When this happens do we really want to stop the device or we want to reschedule a task ?

Yes, I agree with you. If something unexpected occurred, I suppose it would be better to stop the client than prolonging the lives of the process/task.

2. Is there something else to do than stopping the client ? in other word do we really need a callback ?

This is a good point. I focused on keeping the backward compatibility for the existing client implementation, so I intended to leave users to control the behavior to handle something in an unexpected situation, as a callback.
Anyway, I think "leshan-clinet" should stop itself if it falls into an unhealthy status, originally.

3. if we need a callback maybe we can reuse LwM2mClientObserver ?

Ah, yes. I think we can reuse LwM2mClientObserver by extending that interface as you pointed out. Thank you.

4. Looking at PR Add shutdownTrigger for registration engine #928, it seems you need to execute code in LwM2mObjectEnabler too ?

I'm sorry, I'm not sure where should we add the code execution in LwM2mObjectEnabler / ObjectEnabler. Could you tell me more detail about this?

[sbernard31]

I'm sorry, I'm not sure where should we add the code execution in LwM2mObjectEnabler / ObjectEnabler. Could you tell me more detail about this?

I suppose If you ask that means you are motivated to provide contribution ?

If I'm right please could you do that in several PRs.
I think this could be done in this order :

1. We add the unexpectedError(or any better name) callback in LwM2mClientObserver and call it in DefaultRegistrationEngine
2. We use this callback in LeshanClient to destroy it.
3. We add a destroy() method in LwM2mObjectEnabler and call it for each objectEnabler in LeshanClient.destroy().

About 3, maybe better we could reuse Destroyable and Stoppable and in LeshanClient.destroy() and LeshanClient.stop() check if objectEnabler implements this methods like in LeshanServer ?
(if we choose this way, we need to move Destroyable and Stoppable from leshan-core-server to leshan-core, in this case the move you be done in a dedicated commit)

[moznion]

Thank you for your information!
Yeah, I'd like to contribute to this project 😃

About 3, maybe better we could reuse Destroyable and Stoppable and in LeshanClient.destroy() and LeshanClient.stop() check if objectEnabler implements this methods like in LeshanServer ?

Reusing Destroyable and Stoppable looks very good, I love to!

3. We add a destroy() method in LwM2mObjectEnabler and call it for each objectEnabler in LeshanClient.destroy().

I have a question about this. Is it prefer to call ObjectEnabler#destroy() in LeshanClient.destroy() for each ObjectEnabler ? I wonder if this would be better to make it controllable by the user through the callback instead of implicit chained destroying, like the following pseudo code:

public void callback() {
    final LeshanClient client = getClient();
    client.destroy(true);

    // Control the objectEnabler lifecycle by yourself.
    final LwM2mObjectTree objectTree = client.getObjectTree();
    objectTree.getObjectEnablers().forEach(objectEnabler -> {
        objectEnabler.destroy(true);
    });
}

[sbernard31]

Yeah, I'd like to contribute to this project 😃

🎉

I wonder if this would be better to make it controllable by the user through the callback instead of implicit chained destroying, like the following pseudo code.

By "controllable" you mean "user is able to destroy or not" ?
My idea was: if you need a call back to clean-up, you implement Destroyable else you don't implement it ?
or I missed something ?

2 little advises :

• It seems you mainly need Destroyable(It's OK if you only support this one), if you decide to also implement Stoppable you should also support Startable. (it works together because the only purpose of stop is to be able to restart)
• adding a methods on LwM2mObjectTree then do the loop in ObjectTree is maybe better 🤔

[moznion]

Thank you for the comment.

My idea was: if you need a call back to clean-up, you implement Destroyable else you don't implement it ?
or I missed something ?

Probably no, that's true.
My point was "who should take the responsibility to call the LwM2mObjectEnabler#destroy()".

We add a destroy() method in LwM2mObjectEnabler and call it for each objectEnabler in LeshanClient.destroy().

When I read this comment, I thought "LeshanClient.destroy() calls LwM2mObjectEnabler#destroy() in chains". So I wondered which way is better; destroy LwM2mObjectEnablers in LeshanClient.destroy() or in a callback.

However, as you mentioned, it is enough to implement destroy() (or to implement "empty" destroy()) on concrete LwM2mObjectEnabler to control to destroy or not.

Then, I have a doubt: what should be destroy() for existing LwM2mObjectEnabler implementations (i.e. ObjectEnabler) ?

[sbernard31]

I think you don't get my idea or least not exactly what I have in mind.

My point was you implement Destroyable only if you need to destroy().
And in the chain loop you do something like :

  for each LwM2mObjectEnabler
      if LwM2mObjectEnabler implements Destroyable
           LwM2mObjectEnabler.destroy()

[moznion]

Ah, finally I've got it!
You mean, concrete LwM2mObjectEnabler implementation which wants to destroy has to implement like PleaseDestroyMeObjectEnabler implements LwM2mObjectEnabler, Destroyable and filter that by instanceof, right?
I've not had an idea to filter by instanceof in a loop, sorry.

[sbernard31]

Then, I have a doubt: what should be destroy() for existing LwM2mObjectEnabler implementations (i.e. ObjectEnabler) ?

With the solution proposed above, LwM2mObjectEnabler does not implement destroyable.
And ObjectEnabler implement it and the code is the same kind of chain loop on Map<Integer, LwM2mInstanceEnabler> instances attribute.

It's an idea. I don't know if this is the best one 😅.
It seems to me that could do the job.

[sbernard31]

You mean, concrete LwM2mObjectEnabler implementation which wants to destroy has to implement like PleaseDestroyMeObjectEnabler implements LwM2mObjectEnabler, Destroyable and filter that by instanceof, right?

That's the idea :)

[moznion]

Thank you. I'm sorry for bothering you because of my misunderstanding.

I think that idea is great to introduce the new mechanism to destroy, stop and start with keeping the backward compatibility. This is what we needed definitely ✨

So can I start to implement this feature?

[sbernard31]

So can I start to implement this feature?

🚀 !

Please as I said before if you can split it in several PR, this would be better for review :)

[moznion]

Please as I said before if you can split it in several PR, this would be better for review :)

I understand that! Thank you for your help 🙏

[sbernard31]

Just a last question you target Leshan 2.x/master (LWM2M 1.1) or Leshan 1.x(LWM2M 1.0) ?

[moznion]

That’s a good question.
Now I’m using the leshan 1.x, so I need this change on 1.x. But I think this should be also implemented in 2.x edge.

[sbernard31]

Working on 1.x will need more works because there is some rules about not breaking API.
So maybe it's easy to first start by master ? then cherry pick commit and adapt it by adding deprecation and backward compatibility code ?

[moznion]

I agree with you! I’m going to start by master at first.

[sbernard31]

@moznion, you finished your work on master right ?
Do you plan to backport this on 1.x, soon ? This could be less easy as you will face API backward compatibility issue.

I list here the issue you should face :

• Destroyable, Startable and Stoppable mus not be moved but duplicated and deprecate the old one.
• You need to create a new LwM2mClientObserver called LwM2mClientObserver2 to be allowed to add onUnexpectedError

[moznion]

In my understanding, I finished my work on the master branch.

Do you plan to backport this on 1.x, soon ?

Yeah, I'd like to backport those features to the 1.x project (actually this is just what I needed 😄 ).
I'm going to try that according to your suggestion.

[sbernard31]

After that I will probably release a 1.3.0 version of Leshan.

[sbernard31]

This should be fixed in master(#940, #941) and 1.x(#945) and will be available in 2.0.0-M2 and 1.3.0.