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

Fix ApplicationListeners being skipped #158

Merged
merged 7 commits into from
Nov 30, 2023

Conversation

phinner
Copy link
Contributor

@phinner phinner commented Nov 29, 2023

Basically, when a ApplicationListener uses Application#removeListener while the application is iterating over the listener seq, the next listener in the iteration is skipped.

This issue hasn't been noticed for so long since the only use of it is within the dispose of Timer.TimerThread, and that it runs before listeners that doesn't use dispose.

But in BE servers, it prevents my plugins from exiting since Timer is used early by BeControl, running before my plugin shutdown listener thus skipping it.

The fix is simply replacing the removed listener by an empty one.

@Anuken
Copy link
Owner

Anuken commented Nov 29, 2023

Isn't this kind of a memory leak? If you call add and remove in a loop, it will OOM eventually.

@phinner
Copy link
Contributor Author

phinner commented Nov 29, 2023

Isn't this kind of a memory leak? If you call add and remove in a loop, it will OOM eventually.

Well, my reasoning was since it is used rarely and also too much changes, it would do the trick, but now that you mention it... I'll get back to it.

@phinner
Copy link
Contributor Author

phinner commented Nov 29, 2023

Okay, if you don't mind making removeListener and addListener non default, what about this ?

public abstract class AbstractApplication implements Application{

    protected final Seq<ApplicationListener> listeners = new Seq<>();
    protected boolean runningListeners = false;
    protected final Seq<ApplicationListener> removing = new Seq<>();

    @Override
    public Seq<ApplicationListener> getListeners(){
        return listeners;
    }

    @Override
    public void addListener(ApplicationListener listener){
        synchronized(listeners){
            listeners.add(listener);
            removing.remove(listener);
        }
    }

    @Override
    public void removeListener(ApplicationListener listener){
        synchronized(listeners){
            if (runningListeners) removing.add(listener);
            else listeners.remove(listener);
        }
    }

    @Override
    public void forEachListener(Cons<ApplicationListener> cons){
        synchronized(listeners){
            runningListeners = true;
            try{
                for(ApplicationListener listener : listeners){
                    if(removing.contains(listener)) continue;
                    cons.get(listener);
                }
            }finally{
                runningListeners = false;
            }
            listeners.removeAll(removing);
            removing.clear();
        }
    }
}

The forEachListener makes sure no funny business occurs when one want to use the listeners.

@phinner
Copy link
Contributor Author

phinner commented Nov 30, 2023

Looks like I made things too complicated again, the solution was simply removing the listener when it's guaranteed the others are not used.

@Anuken Anuken merged commit f1e4bde into Anuken:master Nov 30, 2023
1 check passed
@phinner phinner deleted the fix/remove-listener-shenanigans branch November 30, 2023 13:24
phinner added a commit to xpdustry/imperium that referenced this pull request Nov 30, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants