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

Better handle unexpected error in DefaultRegistrationEngine. #933

Closed
moznion opened this issue Nov 19, 2020 · 21 comments
Closed

Better handle unexpected error in DefaultRegistrationEngine. #933

moznion opened this issue Nov 19, 2020 · 21 comments
Labels
client Impact LWM2M client new feature New feature from LWM2M specification

Comments

@moznion
Copy link
Contributor

moznion commented Nov 19, 2020

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

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 ?

@sbernard31 sbernard31 added the new feature New feature from LWM2M specification label Nov 19, 2020
@sbernard31 sbernard31 changed the title A client process is stuck when an asynchronous task raises an unexpected error Better handle unexpected error in DefaultRegistrationEngine. Nov 19, 2020
@moznion
Copy link
Contributor Author

moznion commented Nov 20, 2020

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.

  1. 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.

  1. 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.

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

sbernard31 commented Nov 20, 2020

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

moznion commented Nov 24, 2020

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!

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

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 🤔

@sbernard31 sbernard31 added the client Impact LWM2M client label Nov 24, 2020
@moznion
Copy link
Contributor Author

moznion commented Nov 24, 2020

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

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

moznion commented Nov 24, 2020

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

sbernard31 commented Nov 24, 2020

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

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

moznion commented Nov 24, 2020

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

sbernard31 commented Nov 24, 2020

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

moznion commented Nov 24, 2020

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

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

@moznion
Copy link
Contributor Author

moznion commented Nov 24, 2020

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

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

moznion commented Nov 24, 2020

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

moznion added a commit to moznion/leshan that referenced this issue Nov 26, 2020
moznion added a commit to moznion/leshan that referenced this issue Nov 27, 2020
Move the interfaces `Destroyable`, `Startable` and `Stoppable` to
`leshan-core` package from `leshan-server-core`.
This commit aims to use these interfaces with `LwM2mObjectEnabler`
to control the state of that.

Ref: eclipse-leshan#933 (comment)

Signed-off-by: moznion <[email protected]>
sbernard31 pushed a commit that referenced this issue Nov 27, 2020
Move the interfaces `Destroyable`, `Startable` and `Stoppable` to
`leshan-core` package from `leshan-server-core`.
This commit aims to use these interfaces with `LwM2mObjectEnabler`
to control the state of that.

Ref: #933 (comment)

Signed-off-by: moznion <[email protected]>
sbernard31 pushed a commit that referenced this issue Nov 27, 2020
Now it calls `destroy()`, `stop()` and `start()` for each
LwM2mObjectEnabler instance according to the LeshanClient status.
For example, If it calls `LeshanClient#destroy()`, that method
calls for each LwM2mObjectEnabler's `#destroy()`.
And other methods are also similar.

Signed-off-by: moznion <[email protected]>
moznion added a commit to moznion/leshan that referenced this issue Nov 30, 2020
…2mClientObserver

Add an interface method `void onUnexpectedErrorOccurred(Throwable unexpectedError)`
into LwM2mClientObserver. This aims to hook a procedure when an unexpected
error has been occurred.

Signed-off-by: moznion <[email protected]>
moznion added a commit to moznion/leshan that referenced this issue Nov 30, 2020
…ngine`

Call `LwM2mClientObserver#onUnexpectedErrorOccurred()` on
`DefaultRegistrationEngine` when it raises `RuntimeException`
at task loop.

Signed-off-by: moznion <[email protected]>
moznion added a commit to moznion/leshan that referenced this issue Nov 30, 2020
sbernard31 pushed a commit that referenced this issue Dec 7, 2020
To call `LwM2mInstanceEnabler#stop()`, `LwM2mInstanceEnabler#start()`
and `LwM2mInstanceEnabler#destroy()` on the associated method, for each
enabler if implemented the interface.

Signed-off-by: moznion <[email protected]>
sbernard31 pushed a commit that referenced this issue Dec 7, 2020
@sbernard31
Copy link
Contributor

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

moznion commented Dec 8, 2020

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

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

moznion added a commit to moznion/leshan that referenced this issue Dec 8, 2020
moznion added a commit to moznion/leshan that referenced this issue Dec 8, 2020
…able, Stoppable)

And make a suggestion to use interfaces that are in the `core` package.

Signed-off-by: moznion <[email protected]>
moznion added a commit to moznion/leshan that referenced this issue Dec 8, 2020
…r ObjectEnabler

To call each interface method on corresponded `LwM2mObjectTree` method.

Signed-off-by: moznion <[email protected]>
moznion added a commit to moznion/leshan that referenced this issue Dec 8, 2020
…r LwM2mObjectTree

And call each interface's method at related method of `LeshanClient`;
i.e.  `start()`, `stop()` and `destroy()`.

Signed-off-by: moznion <[email protected]>
moznion added a commit to moznion/leshan that referenced this issue Dec 8, 2020
… unexpected erorr

This interface extends `LwM2mClientObserver` interface.
And make `LwM2mClientObserverAdapter` and `LwM2mClientObserverDispatcher`
implement that new interface.
This doesn't break the backward compatibility because
`LwM2mClientObserver2` is compatible with `LwM2mClientObserver` and each
implementing class conceals the difference between `LwM2mClientObserver`
and `LwM2mClientObserver2`.

Signed-off-by: moznion <[email protected]>
moznion added a commit to moznion/leshan that referenced this issue Dec 8, 2020
…occurred

If a task that are belong to `DefaultRegistrationEngine` raises
unexpected `RuntimeException` and the `observer` member variable
implements `LwM2mClientObserver2` (instead of `LwM2mClientObserver`),
it calls `LwM2mClientObserver2#onUnexpectedError()` hook.
The purpose of this hook gimmick is to shutdown the client application
mainly.

Signed-off-by: moznion <[email protected]>
moznion added a commit to moznion/leshan that referenced this issue Dec 8, 2020
…client

And implements `Destroyable` interface for each threading task that
runs with the demo client application.
The reason why it doesn't set the hook at `LeshanClient` is to keep
the backward compatibility, this means it delegates "what should client
do once an unexpected error has occurred" to the users.

Signed-off-by: moznion <[email protected]>
moznion added a commit to moznion/leshan that referenced this issue Dec 8, 2020
moznion added a commit to moznion/leshan that referenced this issue Dec 8, 2020
To clarify the version that is going to remove the deprecated
interfaces.

Signed-off-by: moznion <[email protected]>
sbernard31 pushed a commit that referenced this issue Dec 9, 2020
And make a suggestion to use interfaces that are in the `core` package.

Signed-off-by: moznion <[email protected]>
sbernard31 pushed a commit that referenced this issue Dec 9, 2020
To call each interface method on corresponded `LwM2mObjectTree` method.

Signed-off-by: moznion <[email protected]>
sbernard31 pushed a commit that referenced this issue Dec 9, 2020
And call each interface's method at related method of `LeshanClient`;
i.e.  `start()`, `stop()` and `destroy()`.

Signed-off-by: moznion <[email protected]>
sbernard31 pushed a commit that referenced this issue Dec 9, 2020
This interface extends `LwM2mClientObserver` interface.
And make `LwM2mClientObserverAdapter` and
`LwM2mClientObserverDispatcher`
implement that new interface.
This doesn't break the backward compatibility because
`LwM2mClientObserver2` is compatible with `LwM2mClientObserver` and each
implementing class conceals the difference between `LwM2mClientObserver`
and `LwM2mClientObserver2`.

Signed-off-by: moznion <[email protected]>
sbernard31 pushed a commit that referenced this issue Dec 9, 2020
If a task that are belong to `DefaultRegistrationEngine` raises
unexpected `RuntimeException` and the `observer` member variable
implements `LwM2mClientObserver2` (instead of `LwM2mClientObserver`),
it calls `LwM2mClientObserver2#onUnexpectedError()` hook.
The purpose of this hook gimmick is to shutdown the client application
mainly.

Signed-off-by: moznion <[email protected]>
sbernard31 pushed a commit that referenced this issue Dec 10, 2020
sbernard31 pushed a commit that referenced this issue Dec 10, 2020
And make a suggestion to use interfaces that are in the `core` package.

Signed-off-by: moznion <[email protected]>
Also-by: Simon Bernard <[email protected]>
sbernard31 pushed a commit that referenced this issue Dec 10, 2020
To call each interface method on corresponded `LwM2mObjectTree` method.

Signed-off-by: moznion <[email protected]>
sbernard31 pushed a commit that referenced this issue Dec 10, 2020
And call each interface's method at related method of `LeshanClient`;
i.e.  `start()`, `stop()` and `destroy()`.

Signed-off-by: moznion <[email protected]>
sbernard31 pushed a commit that referenced this issue Dec 10, 2020
This interface extends `LwM2mClientObserver` interface.
And make `LwM2mClientObserverAdapter` and
`LwM2mClientObserverDispatcher`
implement that new interface.
This doesn't break the backward compatibility because
`LwM2mClientObserver2` is compatible with `LwM2mClientObserver` and each
implementing class conceals the difference between `LwM2mClientObserver`
and `LwM2mClientObserver2`.

Signed-off-by: moznion <[email protected]>
sbernard31 pushed a commit that referenced this issue Dec 10, 2020
If a task that are belong to `DefaultRegistrationEngine` raises
unexpected `RuntimeException` and the `observer` member variable
implements `LwM2mClientObserver2` (instead of `LwM2mClientObserver`),
it calls `LwM2mClientObserver2#onUnexpectedError()` hook.
The purpose of this hook gimmick is to shutdown the client application
mainly.

Signed-off-by: moznion <[email protected]>
sbernard31 pushed a commit that referenced this issue Dec 10, 2020
@sbernard31
Copy link
Contributor

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
client Impact LWM2M client new feature New feature from LWM2M specification
Projects
None yet
Development

No branches or pull requests

2 participants