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.

Sign up for GitHub

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

Add callback mechanism into LwM2mClientObserver for when the unexpected error occurred #941

Closed
wants to merge 7 commits into from
Closed

Add callback mechanism into LwM2mClientObserver for when the unexpected error occurred #941

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
2e3a324
#933: Add method: `onUnexpectedErrorOccurred()` for LwM2mClientObserver
moznion Nov 30, 2020
7e32bf5
#933: Handle unexpected errors on `DefaultRegistrationEngine`
moznion Nov 30, 2020
e3aad0d
#933: Add demo code to shutdown when an unexpected error has occurred
moznion Nov 30, 2020
16c6cb7
#933: Rename: s/onUnexpectedErrorOccurred/onUnexpectedError/g
moznion Dec 3, 2020
3f73020
#933: Implement Destroyable, Startable and Stoppable on ObjectEnabler
moznion Dec 3, 2020
7e6d029
#933: register ShutdownOnUnexpectedErrorObserver for LeshanClient
moznion Dec 3, 2020
b4a89c9
#933: format code
moznion Dec 3, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions ...client-core/src/main/java/org/eclipse/leshan/client/engine/DefaultRegistrationEngine.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -526,6 +526,7 @@ public void run() {
LOG.info("Bootstrap task interrupted. ");
} catch (RuntimeException e) {
LOG.error("Unexpected exception during bootstrap task", e);
observer.onUnexpectedErrorOccurred(e);
}
}
}
Expand Down Expand Up @@ -564,6 +565,7 @@ public void run() {
LOG.info("Registration task interrupted. ");
} catch (RuntimeException e) {
LOG.error("Unexpected exception during registration task", e);
observer.onUnexpectedErrorOccurred(e);
}
}
}
Expand Down Expand Up @@ -612,6 +614,7 @@ public void run() {
LOG.info("Registration update task interrupted.");
} catch (RuntimeException e) {
LOG.error("Unexpected exception during update registration task", e);
observer.onUnexpectedErrorOccurred(e);
}
}
}
Expand Down
4 changes: 4 additions & 0 deletions leshan-client-core/src/main/java/org/eclipse/leshan/client/observer/LwM2mClientObserver.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -69,4 +69,8 @@ void onDeregistrationFailure(ServerIdentity server, DeregisterRequest request, R
String errorMessage, Exception cause);

void onDeregistrationTimeout(ServerIdentity server, DeregisterRequest request);

// ============== Unexpected Error Handling =================

void onUnexpectedErrorOccurred(Throwable unexpectedError);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe onUnexpectedError is enough ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for the advice, I've not had confidence in this honestly :)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed at 16c6cb7

}
4 changes: 4 additions & 0 deletions ...ent-core/src/main/java/org/eclipse/leshan/client/observer/LwM2mClientObserverAdapter.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -95,4 +95,8 @@ public void onDeregistrationFailure(ServerIdentity server, DeregisterRequest req
@Override
public void onDeregistrationTimeout(ServerIdentity server, DeregisterRequest request) {
}

@Override
public void onUnexpectedErrorOccurred(Throwable unexpectedError) {
}
}
6 changes: 6 additions & 0 deletions ...-core/src/main/java/org/eclipse/leshan/client/observer/LwM2mClientObserverDispatcher.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -151,4 +151,10 @@ public void onDeregistrationTimeout(ServerIdentity server, DeregisterRequest req
}
}

@Override
public void onUnexpectedErrorOccurred(Throwable unexpectedError) {
for (LwM2mClientObserver observer : observers) {
observer.onUnexpectedErrorOccurred(unexpectedError);
}
}
}
34 changes: 32 additions & 2 deletions leshan-client-demo/src/main/java/org/eclipse/leshan/client/demo/LeshanClientDemo.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,6 +59,7 @@
import org.eclipse.leshan.client.californium.LeshanClientBuilder;
import org.eclipse.leshan.client.engine.DefaultRegistrationEngineFactory;
import org.eclipse.leshan.client.object.Server;
import org.eclipse.leshan.client.observer.LwM2mClientObserverAdapter;
import org.eclipse.leshan.client.resource.LwM2mObjectEnabler;
import org.eclipse.leshan.client.resource.ObjectsInitializer;
import org.eclipse.leshan.client.resource.listener.ObjectsListenerAdapter;
Expand Down Expand Up @@ -623,9 +624,11 @@ public static void createAndStartClient(String endpoint, String localAddress, in
initializer.setInstancesForObject(SERVER, new Server(123, lifetime));
}
}
initializer.setInstancesForObject(DEVICE, new MyDevice());
final MyDevice myDevice = new MyDevice();
final RandomTemperatureSensor randomTemperatureSensor = new RandomTemperatureSensor();
initializer.setInstancesForObject(DEVICE, myDevice);
initializer.setInstancesForObject(LOCATION, locationInstance);
initializer.setInstancesForObject(OBJECT_ID_TEMPERATURE_SENSOR, new RandomTemperatureSensor());
initializer.setInstancesForObject(OBJECT_ID_TEMPERATURE_SENSOR, randomTemperatureSensor);
List<LwM2mObjectEnabler> enablers = initializer.createAll();

// Create CoAP Config
Expand Down Expand Up @@ -745,6 +748,9 @@ public void handshakeFailed(Handshaker handshaker, Throwable error) {
builder.setBootstrapAdditionalAttributes(bsAdditionalAttributes);
final LeshanClient client = builder.build();

client.addObserver(
new ClientShutdownOnUnexpectedErrorObserver(client, myDevice, randomTemperatureSensor));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be done on LeshanClient not LeshanClientDemo or I missed something ?
(#933 (comment))

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, that's a point. I think it would be better to implement this shutdown observer mechanism on LeshanClient, but it breaks the backward compatibility of the client, so I delegated this behavior to the client of the client.

What do you think about this?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For master, we don't care about backward compatibility question and should just focus on what could be the better behavior. (of course this doesn't mean we can break everything without any reason 😅 )

For 1.x, we should try to not break the API and behavior but here we could almost consider this as a bug ... so changing this behavior doesn't disturb me.

WDYT ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I'm on the same page.
I also think it would be better to implement this on the LeshanClient.
I was scared to break the backward compatibility way too much, but it seems no problem. I'll do this.

One more thing. Would it useful to support a method to prevent shutting down even if an unexpected error has occurred on the LeshanClient? For example LeshanClient#doNotDestroyOnUnexpectedError().

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would not add it for now. Let's see if someone need it later.

Copy link
Contributor

@sbernard31 sbernard31 Dec 2, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I still don't see what could be the use case. As tasks in DefaultRegistrationEngine are stopped. 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah, my suggestion was for just in case. It looks there is no reason to support that. Thank you.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed at 7e6d029


client.getObjectTree().addListener(new ObjectsListenerAdapter() {
@Override
public void objectRemoved(LwM2mObjectEnabler object) {
Expand Down Expand Up @@ -872,4 +878,28 @@ public void run() {
}
}
}

private static class ClientShutdownOnUnexpectedErrorObserver extends LwM2mClientObserverAdapter {
final LeshanClient client;
final MyDevice myDevice;
final RandomTemperatureSensor randomTemperatureSensor;

public ClientShutdownOnUnexpectedErrorObserver(
final LeshanClient client,
final MyDevice myDevice,
final RandomTemperatureSensor randomTemperatureSensor
) {
this.client = client;
this.myDevice = myDevice;
this.randomTemperatureSensor = randomTemperatureSensor;
}

@Override
public void onUnexpectedErrorOccurred(Throwable unexpectedError) {
LOG.error("unexpected error occurred", unexpectedError);
client.destroy(true);
myDevice.destroy();
randomTemperatureSensor.destroy();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we need to do the same Lwm2mInstanceEnabler should also implement (startable, stopable, destroyable)
And Object enabler should do pretty much the same as "LwM2mObjectTree" by interating over instance. (see #933 (comment))

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds good. I'll do this 👍

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed at 3f73020

}
}
}
12 changes: 10 additions & 2 deletions leshan-client-demo/src/main/java/org/eclipse/leshan/client/demo/MyDevice.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@

import org.eclipse.leshan.client.resource.BaseInstanceEnabler;
import org.eclipse.leshan.client.servers.ServerIdentity;
import org.eclipse.leshan.core.Destroyable;
import org.eclipse.leshan.core.model.ObjectModel;
import org.eclipse.leshan.core.model.ResourceModel.Type;
import org.eclipse.leshan.core.node.LwM2mResource;
Expand All @@ -23,17 +24,19 @@
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;

public class MyDevice extends BaseInstanceEnabler {
public class MyDevice extends BaseInstanceEnabler implements Destroyable {

private static final Logger LOG = LoggerFactory.getLogger(MyDevice.class);

private static final Random RANDOM = new Random();
private static final List<Integer> supportedResources = Arrays.asList(0, 1, 2, 3, 9, 10, 11, 13, 14, 15, 16, 17, 18,
19, 20, 21);

private final Timer timer;

public MyDevice() {
// notify new date each 5 second
Timer timer = new Timer("Device-Current Time");
this.timer = new Timer("Device-Current Time");
timer.schedule(new TimerTask() {
@Override
public void run() {
Expand Down Expand Up @@ -210,4 +213,9 @@ private long getMemoryTotal() {
public List<Integer> getAvailableResourceIds(ObjectModel model) {
return supportedResources;
}

@Override
public void destroy() {
timer.cancel();
}
}
8 changes: 7 additions & 1 deletion leshan-client-demo/src/main/java/org/eclipse/leshan/client/demo/RandomTemperatureSensor.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,14 +11,15 @@

import org.eclipse.leshan.client.resource.BaseInstanceEnabler;
import org.eclipse.leshan.client.servers.ServerIdentity;
import org.eclipse.leshan.core.Destroyable;
import org.eclipse.leshan.core.model.ObjectModel;
import org.eclipse.leshan.core.response.ExecuteResponse;
import org.eclipse.leshan.core.response.ReadResponse;
import org.eclipse.leshan.core.util.NamedThreadFactory;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;

public class RandomTemperatureSensor extends BaseInstanceEnabler {
public class RandomTemperatureSensor extends BaseInstanceEnabler implements Destroyable {

private static final Logger LOG = LoggerFactory.getLogger(RandomTemperatureSensor.class);

Expand Down Expand Up @@ -113,4 +114,9 @@ private void resetMinMaxMeasuredValues() {
public List<Integer> getAvailableResourceIds(ObjectModel model) {
return supportedResources;
}

@Override
public void destroy() {
scheduler.shutdown();
}
}