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

Calling returnData in the release method might result in a NullPointerException in AbstractComponent #204

Closed
amischler opened this Issue Oct 12, 2016 · 7 comments

Comments

Projects
None yet
2 participants
@amischler

amischler commented Oct 12, 2016

We need to call a service when a model is released, so we use the following call in the release method of a model :

@Override
public boolean release() {
    returnData(WebcamServiceImpl.class, WebcamServiceImpl.CLOSE, Builders.waveData(WebcamServiceImpl.WEBCAM, currentWebcam));       
    return super.release();
}

The call to returnData will create an AbstractJrbRunnable of priority Normal which will be executed later in the JIT. The call to super.release will also create an AbstractJrbRunnable of priority Normal to be executed in the JIT in order to release the model.

Both runnables are registered in the processingTasks of the JRebirthThread. According to the PriorityBlockingQueue, the order of execution of two tasks with the same priority is not guaranteed :

Operations on this class make no guarantees about the ordering of elements with equal priority.

So it might happen that the release runnable get executed before the runnable submitted by returnData which will fail since getNotifier() of the released component will return null :

java.lang.NullPointerException: null
    at org.jrebirth.af.core.component.basic.AbstractComponent.getNotifier(AbstractComponent.java:110) ~[core-8.0.5.jar:na]
    at org.jrebirth.af.core.component.basic.AbstractComponent.access$000(AbstractComponent.java:81) ~[core-8.0.5.jar:na]
    at org.jrebirth.af.core.component.basic.AbstractComponent$3.runInto(AbstractComponent.java:284) ~[core-8.0.5.jar:na]
    at org.jrebirth.af.core.concurrent.AbstractJrbRunnable.run(AbstractJrbRunnable.java:80) ~[core-8.0.5.jar:na]
    at org.jrebirth.af.core.concurrent.JRebirthThread.run(JRebirthThread.java:170) ~[core-8.0.5.jar:na]
@sbordes

This comment has been minimized.

Member

sbordes commented Oct 12, 2016

Perhaps you could refactor your code like this:

Update the WaveType WebcamServiceImpl.CLOSE to send a wave (using the famous voidItem)
Listen this return wave type into your model and update the release sequence

    public void onClose(Wave wave){
        webcamOn = false;
        release();
    }

    @Override
    public boolean release() {
        if(webcamOn){
            returnData(WebcamServiceImpl.class, WebcamServiceImpl.CLOSE, Builders.waveData(WebcamServiceImpl.WEBCAM, currentWebcam));       
        } else {
            return super.release();
        }
    }

Another option would be to use JRebirth.run***Sync methods, but it will be more touchy to do with ServiceTask

Does it make the job ?

@sbordes sbordes added the enhancement label Oct 12, 2016

@sbordes sbordes added this to the 8.5.1 milestone Oct 12, 2016

@sbordes sbordes self-assigned this Oct 12, 2016

@sbordes

This comment has been minimized.

Member

sbordes commented Oct 12, 2016

In any case I will perform an update to be able to define custom priority when calling attachUi, callCommand and returnData to be sure of the execution order in such cases.
The release will kept its normal priority.

I will probably also add a way to force the execution synchronously.

These two features will be configured using custom WaveData

@amischler

This comment has been minimized.

amischler commented Oct 12, 2016

Thanks but this won't work in my case since the webcam might require to be closed without closing the view and releasing the model. So, for the moment I'm using the following workaround :

@Override
    public boolean release() {
        returnData(WebcamServiceImpl.class, WebcamServiceImpl.CLOSE, Builders.waveData(WebcamServiceImpl.WEBCAM, currentWebcam));
        // workaround for https://github.com/JRebirth/JRebirth/issues/204
        // make sure that we are released after any call to returnData in the stop method by setting a priority of low
        JRebirth.runIntoJIT(new JRebirthRunnable() {
            @Override
            public void run() {
                WebcamModel.super.release();
            }

            @Override
            public RunnablePriority getPriority() {
                return RunnablePriority.Low;
            }
        });
        return true;
    }

This way, I'm sure that the release runnable with a priority of low won't be executed before the return data runnable.

@sbordes

This comment has been minimized.

Member

sbordes commented Oct 12, 2016

In my code sample the view isn't released the first time but after the callback of your WebcamServiceImpl, so it should work, nevertheless your patch works and you will have to do the inverse when I will add priority management (by adding Priority.High to your returnData call).

I will let the possibility to change the release priority too by using a local properties map per component #160.

The fix will be based on 8.5.0 version so you will have to upgrade your code, some refactoring have been done and a real modular approach is ready to manage Interface+Implementation.

@amischler

This comment has been minimized.

amischler commented Oct 12, 2016

I think this is interesting to be able to set a custom priority in the attachUi, callCommand and returnData. However, I'm afraid that it won't be that easy as a developer to anticipate such issues and to know when it will be required to set a custom priority when using these methods.

More generally, I think that it would be interesting to improve how JRebirth handle released models :

We had several issues related to NPE in JRebirth models and most of the time it was due to the fact that a release model is still trying to use waves or facades, etc... this might happen for example if a model has registered listeners to other components and these listeners are not removed when releasing. Those listeners might still try to use model features when reacting to future events even if the model is released.

Would it be possible in all those cases that JRebirth does not fail with an NPE (because the facade or notifier is null) but with a more precise message indicating that the model is released and should not be use anymore ? And maybe to add utilities methods in the Model class helping to register listeners that will be automatically removed when the model is released ? Someting like :

public void registerListener(ObservableValue<T> observable, ChangeListener<T> listener) {
    observable.addListener(listener);
    observables.put(observable, listener);
}

The model could store the registered listeners and remove them on release.

What's your opinion about this and how to prevent such cases ?

@sbordes

This comment has been minimized.

Member

sbordes commented Oct 12, 2016

The public release method shall be overridden to handle all these custom action. It should be the way when you add custom listeners to a component.
However we can probably solve some problem for wave type listening and other things directly linked to JRebirth internals. I will create another issue to track this request fro enhancement.

@sbordes sbordes closed this in f80b2ff Oct 12, 2016

@sbordes

This comment has been minimized.

Member

sbordes commented Oct 12, 2016

With 8.5.1-SNAPSHOT Use this sample code

@Override
public boolean release() {
    returnData(WebcamServiceImpl.class,
                     WebcamServiceImpl.CLOSE, 
                     WBuilder.waveData(WebcamServiceImpl.WEBCAM, currentWebcam),
                     WBuilder.priority(PriorityLevel.HIGH)
                   );
    return super.release();
}
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment