Skip to content
This repository has been archived by the owner on May 7, 2020. It is now read-only.

fix illegal status transitions if handler missing #4182

Closed
wants to merge 2 commits into from
Closed

fix illegal status transitions if handler missing #4182

wants to merge 2 commits into from

Conversation

J-N-K
Copy link
Contributor

@J-N-K J-N-K commented Sep 3, 2017

fixes #942

Signed-off-by: Jan N. Klug [email protected]

Copy link
Contributor

@maggu2810 maggu2810 left a comment

Choose a reason for hiding this comment

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

Hm, a binding could depend on an OSGi service that is not present at the moment and so there will be no handler available.
Shouldn't the UI (e.g. PaperUI) be changed so if the handler is missing and a deletion would assume that all resources has been already freed, a notification is displayed first that needs to be accepted?
Currently I will "see" that the removal is not done and I need to press again and accept the "hard" removal.

I don't know if this is critical and a resource management in a handler could rely on the fact that it handles every removal...

@J-N-K
Copy link
Contributor Author

J-N-K commented Sep 4, 2017

But: If no handler is present, how should it handle the removal? IMO all handlers should make sure that they clean up everything during dispose(). Who should track ressources that are used by non-existing or no-longer-existing handlers?

@maggu2810
Copy link
Contributor

I a handler is not present and I try to delete a thing using the Paper UI thecurrent behavior is that it first change the state to removing. Then I have to press "delete" again and a message like this is shown:

Remove Thing ...
Would you like to remove the thing from the system?

Warning: The thing is currently in the removing state and the binding has not yet confirmed, that the device has been removed successfully. If you now click on remove, it will be removed without additional communication to the binding.

If I still want to remove the thing, then I have to press "force remove".

This is the current situation.

Now you change the behavior that if a handler is missing no message is shown in the PaperUI anymore but the thing will be removed without any further confirmation.

My question has been, if your intention is to drop the "it will be removed without additional communication to the binding", too or if this is a side effect that you don't care.
Has this changed behavior (I assume it is changed) discussed already?

@sjsf
Copy link
Contributor

sjsf commented Sep 4, 2017

IMO all handlers should make sure that they clean up everything during dispose()

That's true for stuff like file handles and the like only. However, there might be some persistent stuff that handlers would want to tidy up, e.g. encryption keys which need to be removed from devices. Users might be forced to factory-reset their devices otherwise if they will every want to use them again. Therefore, depending on the device types, the force-remove indeed can be a nasty thing to do.

Therefore, I'm afraid this is fixing it at the wrong place. I would expected a proper remove handling by the ThingHandler once it's there. So the problem is that the ThingManager actually tries to initialize a thing in REMOVING status. The ThingHandler obviously needs to get initialized in order to be able to communicate with the device, but the ThingStatus should not go the normal route (maybe by injecting a different ThingHandlerCallback?) but rather call the handleRemove() once it got (silently) initialized. I have to admit it's a little tricky...

For lifecycle-stuff like this here, please also add a small test-case to the ThingManagerOSGiTest. The ThingManager is a beast, and I'm afraid we will lose overview otherwise and run into regressions all the time.

@J-N-K
Copy link
Contributor Author

J-N-K commented Sep 4, 2017

@SJKA, I think a silent initialization is very confusing and unexpected. And it will not solve the problem. If the handler needs to delete encryption keys from the device, it'll probably not be able to do so before the thing is ONLINE and the handler can communicate with the device.

One could/should send a warning to the user that removing things in UNINITIALZED/HANDLER_MISSING state may have side-effects (like we do now, if it is in REMOVING and we try to remove it again).

@sjsf
Copy link
Contributor

sjsf commented Sep 5, 2017

@SJKA, I think a silent initialization is very confusing and unexpected. And it will not solve the problem. If the handler needs to delete encryption keys from the device, it'll probably not be able to do so before the thing is ONLINE and the handler can communicate with the device.

No, you got me wrong, I think. With "silent initialization" I didn't mean to just set the Thing to initialized! Instead I would suggest to instantiate a ThingHandler, inject into it the Thing & a special ThingHandlerCallback implementation and then call ThingHandler.initialize(). The main purpose of the special ThingHandlerCallback is to detect once the ThingHandler has finished the initialization and then call handleRemoval() on it. The Thing stays in REMOVING all the time because the special ThingHandlerCallback doesn't care to set the Thing to anything different if asked by the handler. Maybe just just invent a new REMOVING/HANDLER_INITIALIZATION state for this, to make it more transparent.

Better ideas how to get hold of a fully-working, initialized handler instance which is capable of doing the handleRemoval() are welcome!

One could/should send a warning to the user that removing things in UNINITIALZED/HANDLER_MISSING state may have side-effects (like we do now, if it is in REMOVING and we try to remove it again).

Isn't it what we are doing already today? There is no direct "force-remove" in the PaperUI, right? So the user would have to go through "remove" -> "force-remove" also for UNINITIALIZED things, hence will get presented the warning.

Please have a look at the original issue again (#942). It's about the fact that the framework makes the thing move through REMOVING -> INITIALIZING -> ONLINE, i.e. ignoring the user's intent to make it go away.

@J-N-K
Copy link
Contributor Author

J-N-K commented Sep 5, 2017

I understood that. But as I said above: probably the thing needs to go ONLINE before it can be removed. Having a thing going through all states from UNINITIALIZED/HANDLER_MISSING to ONLINE silently is a bad move.

I think I didn't make it clear enough what I proposed: Currently we allow "force removal" if the thing is in REMOVING and we try to remove it again. My suggestion is to add a warning "something strange may happen if you remove an uninitialized thing" on the first attempt to remove it and then proceed like we force-removed it.

This would indeed solve #942, because either the thing would not go into REMOVING (because the user stopped because of the warning) or the thing would be removed and cannot go to initialized later.

@triller-telekom
Copy link
Contributor

This would indeed solve #942, because either the thing would not go into REMOVING (because the user stopped because of the warning) or the thing would be removed and cannot go to initialized later.

I agree with you that it will solve #942, but it does it in a wrong way. We should not eliminate the REMOVING state, because this will give a handler the possibility to clean up once the handler is available again. If the user decides to remove the thing nevertheless, i.e. he doesn't like that is is in the REMOVING state, he can still choose to force remove it as it is implemented at the moment.

@J-N-K
Copy link
Contributor Author

J-N-K commented Sep 21, 2017

Then there is no way to fix this.

@J-N-K J-N-K closed this Sep 21, 2017
@kaikreuzer
Copy link
Contributor

I hope what you meant is "Then this PR does not fix this" - or do you mean that there is no way to solve #942?

@J-N-K
Copy link
Contributor Author

J-N-K commented Sep 21, 2017

There is no way to fix #942. If we need an initialized thing handler to go from REMOVING to REMOVED (otherwise dispose may fail, if communication with the device is needed), then how should a thing with HANDLER_MISSING be removed if not by initialization first?

@kaikreuzer
Copy link
Contributor

There is no way to fix #942.

Then this should be discussed on #942.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Thing status changes from REMOVING to INITIALIZING
5 participants