jvermillard / leshan

OMA Lightweight M2M java implementation - LWM2M
40 stars 25 forks source link

Fix handling of missing Endpoint for client communication #19

Closed sophokles73 closed 10 years ago

sophokles73 commented 10 years ago

The getEndpointForClient() threw an InvalidStateExceptionif no Endpoint could be found for the Client. However, this exception had not been caught or otherwise been handled apropriately by any code yet. Thus I changed this to throw a ResouceAccessExceptioninstead which is properly handled.

jvermillard commented 10 years ago

This is really an IllegalStateException because if you can't find the endpoint for a registered client, it's meaning we have leshan inner data structures in bad state and the source of this is a bug. So we are not supposed to catch this exception.

sophokles73 commented 10 years ago

I would agree with you if the only way to set up and configure the underlying Californium server would be by means of starting leshan-standalone. However, we are already working on OSGi support for leshan where the Californium server is provided by means of a ManagedServer by the OSGi container and leshan only binds to this server during start-up. In this case, the ManagedServer (Californium) could very well be re-configured or stopped, thus an Endpoint may disappear during runtime. This case is probably very unlikely but it is still possible. I would thus rather have leshan handle this situation in a graceful way instead of simply throwing an unhandled runtime exception. Does this make any sense? Or is there another way we could handle this case?

jvermillard commented 10 years ago

Thanks for the details, now I understand what you are doing here :) I'm not sure a "resource not found" is a clearer exception. IMO you should try catch the illegal state or clearing the Client registry of client where the endpoint is not anymore present

sophokles73 commented 10 years ago

I am under the very same impression. We could probably let the ResourceAccessException bubble up all the way up the call stack to the client, the LwM2MRequest.send() method actually declares this exception (e.g. thrown when a request times out) so a client should be prepared to handle it. I could define a specific sub-class explicitly indicating this kind of problem. I also like the idea of removing all affected clients from the registry in this case ... what do you think? Go for a ResourceAccessException sub-class?

jvermillard commented 10 years ago

I would agree to go for the typed exception if the actual code of leshan-core would be able to hot change the underlying californium endpoint. For now it would just creep the API. Another solution, before changing your endpoints, walk the client list and kill the session with endpoints which are going to be removed.