ConcurrentModificationException

[temi]

Hallo zusammen,
mein erster Post in diesem Forum
Ich habe in der Vergangenheit bereits mit Basic, Pascal, Delphi (sehr lange her) und später mit C# programmiert. Da ich zuhause seit einiger Zeit ausschließlich unter Linux arbeite, wollte ich mich nun noch einmal mit einer Sprache beschäftigen, die auch hier gut unterstützt wird. Nach einem kurzen Schnupperausflug in C++ bin ich jetzt bei Java gelandet. Ist alles nur hobbymäßig, um der frühzeitigen Vergreisung entgegen zu wirken.

Aktuell versuche ich mir ein kleines Messagesystem zu schreiben, d.h. Objekte können sich registrieren und alle Objekte, die eine Methode "handle(Event e)" implementieren werden bei einem "dispatch(Event e)" benachrichtigt. Das hat auch schon ganz gut funktioniert, bis ich den festen Namen des Handlers flexibel gestalten wollte. Jetzt erhalte ich die oben genannte Ausnahme, wenn ich ein Objekt mit "unregister" wieder entfernen möchte.

Was mache ich dabei falsch?
Und ganz allgemein: Ist das alles einigermaßen in Ordnung, oder gibt es grobe Schnitzer?

Dankeschön,
temi

import java.util.List;
import java.util.ArrayList;

public class EventAggregator
{
    private class Handler
    {
        private final Object handler;
        private final String method;

        public Handler(Object handler, String method)
        {
            this.handler = handler;
            this.method = method;
        }

        public Object getHandler()
        {
            return handler;
        }

        public String getMethod()
        {
            return method;
        }
    }

    private List<Handler> handlers = new ArrayList<>();

    public void register(Object handler)
    {
        this.register(handler, "handle");
    }

    public void register(Object handler, String method)
    {
        handlers.add(new Handler(handler, method));
    }

    public void unregister(Object handler)
    {
        for(Handler h : handlers)
        {
            if (h.getHandler() == handler)
            {
                handlers.remove(h); // Hier Exception!
            }
        }
    }

    public void dispatch(Event event)
    {
        for(Handler h : handlers)
        {
            Object handler = h.getHandler();
            String method = h.getMethod();
            try
            {
                handler.getClass()
                       .getDeclaredMethod(method, new Class[]{event.getClass()})
                       .invoke(handler, event);
            }
            catch(Exception e) { }
        }
    }
}

 

[CSHW89]

Die Exception wird immer dann geworfen, wenn du versuchst eine Liste (bzw. allgemein eine Collection) zu verändern, während du über sie iterierst. Das schöne ist aber, ein Iterator besitzt die Methode 'remove' um das aktuelle Element zu löschen:

public void unregister(Object handler) {
    Iterator<Handler> it = handlers.iterator();
    while (it.hasNext()) {
        Handler h = it.next();
        if (h.getHandler() == handler) {
            it.remove();
        }
    }
}

Grüße
Kevin

 

[mrBrown]

Was mache ich dabei falsch?

Währende eines for-each-Loops darf man die Liste nicht verändern

Eine Möglichkeit wäre direkt mir Iterator zu arbeiten, die schönere wäre removeIf:

handlers.removeIf((Handler h) -> h.getHandler() == handler);

Und ganz allgemein: Ist das alles einigermaßen in Ordnung, oder gibt es grobe Schnitzer?

MMn ist das so durchaus in Ordnung - bis auf den leeren catch-Block, der ist ganz Böse

Ich persönlich würd aber statt Reflection den Typsicheren Weg gehen, und statt Methodenname ein Consumer<Event> nutzen.

 

[temi]

Zunächst mal vielen Dank euch beiden. Die Lösung von CSHW89 funktioniert schon mal. Die Lösung von mrBrown sicherlich auch, das muss ich mir noch mal näher anschauen.

MMn ist das so durchaus in Ordnung - bis auf den leeren catch-Block, der ist ganz Böse

Was wäre eine bessere Lösung dafür? "getDeclaredMethod" wirft eine Exception, falls die gesuchte Methode nicht existiert und ich kenne keine andere Unterscheidungsmöglichkeit.

Ich persönlich würd aber statt Reflection den Typsicheren Weg gehen, und statt Methodenname ein Consumer<Event> nutzen.

Ich verstehe leider nicht ganz was du meinst, aber ursprünglich wollte ich so etwas haben:

class Foo implements Event<EventA>
{
    //...
    public void handle(EventA e) {}
}

Aber das scheitert, wenn eine Klasse auf mehrere Events reagieren soll, wegen dem Verhalten von Generics in Java.

 

[mrBrown]

Was wäre eine bessere Lösung dafür? "getDeclaredMethod" wirft eine Exception, falls die gesuchte Methode nicht existiert und ich kenne keine andere Unterscheidungsmöglichkeit.

Reagiere einfach auf die Exception, und wenn es nur Loggen ist, damit der Nutzer zumindest weiß, was er Falsch gemacht hat.
Was soll man da unterscheiden?

Ich verstehe leider nicht ganz was du meinst, aber ursprünglich wollte ich so etwas haben:

class Foo implements Event<EventA>
{
    //...
    public void handle(EventA e) {}
}

Aber das scheitert, wenn eine Klasse auf mehrere Events reagieren soll, wegen dem Verhalten von Generics in Java.

class Foo
{
    //...
    public void handle(EventA e) {}
}

public void register(Consumer<EventA> handler)
    {
        //...
    }

//Mit Methodenreferenz
eventAggregator.register(foo::handle);

//Mit Lambda
eventAggregator.register((EventA event) -> foo.handle(event));

 

[temi]

Reagiere einfach auf die Exception, und wenn es nur Loggen ist, damit der Nutzer zumindest weiß, was er Falsch gemacht hat.
Was soll man da unterscheiden?

Ich verstehe was du meinst, aber der Nutzer hat ja nichts falsch gemacht, es gibt schlicht nur keinen Handler für ein bestimmtes Event. Mit unterscheiden meinte ich: Ich gehe durch alle registrierten Handler und prüfe, ob ein Handler auf ein bestimmtes Ereignis reagieren kann. Falls ja, dann wird die entsprechende Methode aufgerufen, andernfalls - passiert nichts. Die Anregung mit dem Logging werde ich auf die todo-Liste setzen, wobei bei 100 registrierten Handlern, von denen einer auf ein bestimmtes Ereignis reagiert, dann 99 Logeinträge erscheinen würden...

Die andere Sache muss ich mir noch mal durch den Kopf gehen lassen und noch etwas recherchieren. Es sei denn du hast Zeit und Lust das noch etwas deutlicher auszuführen.
Mit Lambdas muss ich noch warm werden und "foo::handle" - was bewirkt das? Ich kenne sowas aus C++, aber wusste nicht, dass es das auch in Java gibt.

 

[mrBrown]

Ich verstehe was du meinst, aber der Nutzer hat ja nichts falsch gemacht, es gibt schlicht nur keinen Handler für ein bestimmtes Event. Mit unterscheiden meinte ich: Ich gehe durch alle registrierten Handler und prüfe, ob ein Handler auf ein bestimmtes Ereignis reagieren kann. Falls ja, dann wird die entsprechende Methode aufgerufen, andernfalls - passiert nichts. Die Anregung mit dem Logging werde ich auf die todo-Liste setzen, wobei bei 100 registrierten Handlern, von denen einer auf ein bestimmtes Ereignis reagiert, dann 99 Logeinträge erscheinen würden...

Dann solltest du NoSuchMethodException explizit nicht behandeln und deutlich machen warum und alle anderen Exceptions separat - Exception catchen ist in jedem Fall schlechter Stil

Mit Lambdas muss ich noch warm werden und "foo::handle" - was bewirkt das? Ich kenne sowas aus C++, aber wusste nicht, dass es das auch in Java gibt.

Ganz grob gesagt: Es übergibt die Methode handle des Objekts foo an die Methode.

Die Methode erwartet einen Consumer<EventA>, und Consumer haben eine Methode, die als einziges Argument ein EventA bekommt. Überall wo dann ein Consumer erwartet wird, kannst du dann über Methodenreferenz oder Lambda eine Methode übergeben, die die gleichen Argumente wie Consumer bekommt - also EventA. (Rückgabetyp und Exceptions müssen auch passen, beides hat Consumer aber nicht)

 

[temi]

Dann solltest du NoSuchMethodException explizit nicht behandeln und deutlich machen warum und alle anderen Exceptions separat - Exception catchen ist in jedem Fall schlechter Stil

Ist angekommen

 

[temi]

Nur noch eine Frage am Rande:

Macht das irgendeinen Unterschied?

public class EventAggregator
{
    private List<Handler> handlers = new ArrayList<>();
    //...
}

vs

public class EventAggregator
{
    private List<Handler>handlers;
    public EventAggregator()
    {
        handlers = new ArrayList<>();
    }
    //...
}

 

[mrBrown]

Ist nur persönliche Präferenz - ich find ersteres angenehmer.

 

[temi]

Ist nur persönliche Präferenz - ich find ersteres angenehmer.

ACK

 

[AndyJ]

Hi,
das eigentliche Problem ist, dass eine ArrayList nicht threadsafe ist. Statt ArrayList kannst du eine CopyOnWriteArrayList verwenden:

List<Integer> list = new CopyOnWriteArrayList<>();
list.add(1);
list.add(2);
list.add(3);
System.out.println(list);
for (Integer i : list) {
    if (i == 2) {
        list.remove(Integer.valueOf(2)); // keine exception mehr :o)
    }
}
System.out.println(list);
// prints:
// [1, 2, 3]
// [1, 3]

Cheers,
Andy

 

[mrBrown]

das eigentliche Problem ist, dass eine ArrayList nicht threadsafe ist. Statt ArrayList kannst du eine CopyOnWriteArrayList verwenden:

Nö, das Problem ist, dass man in for-each einfach nicht die Liste verändert, sollte man auch mit ThreadSafe-Listen nicht machen. Es gibt schließlich eine extra Funktion dafür...

 

[AndyJ]

Nö, das Problem ist, dass man in for-each einfach nicht die Liste verändert, sollte man auch mit ThreadSafe-Listen nicht machen. Es gibt schließlich eine extra Funktion dafür...

Erstmal: wenn man eine CopyOnWriteArrayList verwendet, wird die Liste ueber die man iteriert NICHT veraendert:

The "snapshot" style iterator method uses a reference to the state of the array at the point that the iterator was created. This array never changes during the lifetime of the iterator, so interference is impossible and the iterator is guaranteed not to throw ConcurrentModificationException.

Den Iterator kann man natuerlich auch verwenden, aber es ist nicht garantiert dass Iterator.remove() tatsaechlich implementiert ist. Das ist naemlich eine optionale Operation, siehe Javadoc.

Wie immer fuehren mehrere Wege nach Rom, z.B. auch dieser hier:

List<Integer> list = new ArrayList<>();
list.add(1);
list.add(2);
list.add(3);
System.out.println(list);
List<Integer> toRemove = new ArrayList<>();
for (Integer i : list) {
    if (i == 2) {
        toRemove.add(i);
    }
}
for (Integer i : toRemove) {
   list.remove(i);
}
System.out.println(list);
// prints:
// [1, 2, 3]
// [1, 3]

Jede Methode hat ihre Vor- und Nachteile und als Programmierer sollte man moeglichst viele kennen und in der Lage sein abzuwaegen, welche die Beste ist. Dogmatismus ist ein schlechter Ratgeber.

Cheers,
Andy

 

[mrBrown]

Erstmal: wenn man eine CopyOnWriteArrayList verwendet, wird die Liste ueber die man iteriert NICHT veraendert:

Ich hab überhaupt nichts dazu gesagt, wie über die Liste iteriert wird...
Dein Beispiel ist und bleibt schlechter Stil, nichts anderes hab ich gesagt.

Den Iterator kann man natuerlich auch verwenden, aber es ist nicht garantiert dass Iterator.remove() tatsaechlich implementiert ist. Das ist naemlich eine optionale Operation, siehe Javadoc.

Deshalb würde ich den auch nicht verwenden und habe ihn auch nicht empfohlen...Der von CopyOnWriteArrayList implementiert remove zB aus offensichtlichen Gründen nicht.

Jede Methode hat ihre Vor- und Nachteile und als Programmierer sollte man moeglichst viele kennen und in der Lage sein abzuwaegen, welche die Beste ist. Dogmatismus ist ein schlechter Ratgeber.

Und noch besser, also viele Wege zu kennen, ist es die dafür gedachte Funktion zu kennen

 

[temi]

Ich habe den EventManager nochmal überarbeitet wollte den aktuellen Stand zwecks Vollständigkeit mal zeigen. Ich bin damit ganz zufrieden, auch wenn ich weiterhin auf Reflection zurück greife. Ich persönlich mag den "convention over configuration" Ansatz und finde die Schnittstelle so einfach und verständlich.

public final class EventManager implements IEventManager
{
    private List<Handler> handlers = new ArrayList<>();

    @Override
    public void subscribe(final Object subscriber)
    {
        if (subscriber == null)
        {
            throw new NullPointerException("Subscriber");
        }

        for (Handler handler : handlers)
        {
            if (handler.weakReference.get().equals(subscriber))
            {
                throw new IllegalArgumentException("Subscriber already exists: '" + subscriber.getClass().getName() + "'");
            }
        }

        handlers.add(new Handler(subscriber));
    }

    @Override
    public void remove(final Object subscriber)
    {
        handlers.removeIf(handler -> handler.weakReference.get().equals(subscriber));
    }

    @Override
    public void publish(final Object event)
    {
        if (event == null)
        {
            throw new NullPointerException("event");
        }

        ArrayList<Handler> toNotify = new ArrayList<>(handlers);

        ArrayList<Handler> dead = new ArrayList<>();

        for (Handler handler : toNotify)
        {
            if (!handler.handle(event))
            {
                dead.add(handler);
            }
        }

        handlers.removeAll(dead);
    }

    /**
     * Internal class wrapping an object to handle events
     */
    private class Handler
    {
        final WeakReference<Object> weakReference;
        Dictionary<Class, Method> supportedEvents = new Hashtable<>();

        /**
         * Creates a new handler
         * @param handler the object handling events. A handler has to implement a
         *                method 'handle(Event e)' for each event.
         * @throws IllegalArgumentException if the handler has not implemented any handling method.
         */
        Handler(final Object handler)
        {
            weakReference = new WeakReference<>(handler);

            Method[] declaredMethods = handler.getClass().getDeclaredMethods();

            for (Method method : declaredMethods)
            {
                if (method.getName().equals(DEFAULT_METHOD))
                {
                    Class[] parameterTypes = method.getParameterTypes();
                    supportedEvents.put(parameterTypes[0], method);
                }
            }

            if (supportedEvents.isEmpty())
                throw new IllegalArgumentException("Method '" + DEFAULT_METHOD + "' is not implemented: '" + handler.getClass().getName() + "'");
        }

        /**
         * Invokes the handling method on the target object.
         * @param event to invoke.
         * @return 'false' if the targeted weak reference is not valid anymore, else 'true'
         */
        boolean handle(final Object event)
        {
            Object target = weakReference.get();

            if (target == null) return false;

            Method handler = supportedEvents.get(event.getClass());

            if (handler != null)
            {
                try
                {
                    handler.invoke(target, event);
                }

                catch (IllegalAccessException e)
                {
                    // todo:
                    System.err.println(e);
                }

                catch (InvocationTargetException e)
                {
                    // todo:
                    System.err.println(e);
                }
            }

            return true;
        }
    }
}

In der Main sieht das dann so aus:

public class Main
{
    public static void main(String[] args)
    {
        IEventManager events = new EventManager();

        // inject EventManager and subscribe internally
        Foo foo = new Foo(events);
        Foo foe = new Foo(events);

        Bar bar = new Bar();
        Baz baz = new Baz(); // Test: This class has no handler implemented

        events.subscribe(bar);
        //events.subscribe(bar); // Test: IllegalArgumentException
        //events.subscribe(baz); // Test: IllegalArgumentException

        events.publish(new TextMessage("lorem ipsum"));
        events.publish(new CounterChanged(42));

        events.remove(baz); // Test: does not exist
        events.remove(foo);

        events.publish(new TextMessage("dolor sit amet"));
        events.publish(new CounterChanged(42));
    }
}

Und Foo und Bar so:

public class Foo
{
    private final IEventManager events;

    public Foo(final IEventManager events)
    {
        Objects.requireNonNull(events, "events");

        this.events = events;

        // Test: subscribe "this"
        events.subscribe(this);
    }

    public void handle(TextMessage event)
    {
        System.out.println(this.getClass().getSimpleName() + ": " + event.getMessage());

        events.publish(new CounterChanged(100));

        // Test: remove "this"
        events.remove(this);
    }
}

public class Bar
{
    public void handle(TextMessage event)
    {
        System.out.println(this.getClass().getSimpleName() + ": " + event.getMessage());
    }

    public void handle(CounterChanged event)
    {
        System.out.println(this.getClass().getSimpleName() + ": " + event.getCount());
    }
}

Meinungen dazu sind mir willkommen, weil ich gerne dazu lernen möchte und erst seit kurzem mit Java arbeite. Deshalb auch dieses kleine Projekt, um mit den Möglichkeiten etwas vertrauter zu werden.

Gruß,
temi

 

[mrBrown]

if (subscriber == null)
        {
            throw new NullPointerException("Subscriber");
        }

Kannst du abkürzen zu Objects.requireNonNull(subscriber, "Subscriber");

handler.weakReference.get()

Würde ich mit einem einfachen Getter ersetzen, z.B. handler.getSubscriber().

Außerdem ist das potentiell für eine NPE anfällig, die prüfst du nirgends.


Das prüfen, ob der Handler noch "lebt", würde ich von handle trennen. Das ließe sich schöner lösen mit einer isAlive() in Handler (was dann natürlich trotzdem nach dem publishen aller Events geprüft werden kann).
Von handle würde ich eher erwarten, dass sie zurückgibt, ob das Event behandelt wurde oder nicht, zB wegen falschen Typen.

 

[temi]

Kannst du abkürzen zu Objects.requireNonNull(subscriber, "Subscriber");

OK

Außerdem ist das potentiell für eine NPE anfällig, die prüfst du nirgends.

NPE = NullPointerException?

Object target = weakReference.get();

            if (target == null) return false;

Prüfe ich doch, oder nicht?
Ah, ich sehe grade, du meinst bei remove(Object subsriber)?
Habe ich jetzt so korrigiert:

@Override
    public void remove(final Object subscriber)
    {
        handlers.removeIf(handler -> handler.exists(subscriber));
    }

boolean exists(Object subscriber)
        {
            Object target = weakReference.get();

            return target == null ? false : target.equals(subscriber);
        }

Das prüfen, ob der Handler noch "lebt", würde ich von handle trennen. Das ließe sich schöner lösen mit einer isAlive() in Handler (was dann natürlich trotzdem nach dem publishen aller Events geprüft werden kann).
Von handle würde ich eher erwarten, dass sie zurückgibt, ob das Event behandelt wurde oder nicht, zB wegen falschen Typen.

Ich habe die Methode in "handleAndIsAlive" umbenannt, um die Rückgabe klar zu stellen und trotzdem alles "in einem Aufwasch" machen zu können.

 

[temi]

Vergessen: Vielen Dank für deine Rückmeldung. Sehr erhellend bisher.

 

[mrBrown]

NPE = NullPointerException?

Ja

Ah, ich sehe grade, du meinst bei remove(Object subsriber)?

Und auch beim hinzufügen

Ich habe die Methode in "handleAndIsAlive" umbenannt, um die Rückgabe klar zu stellen und trotzdem alles "in einem Aufwasch" machen zu können.

Naja, die Methode macht halt immer noch mehrere Dinge - das wäre nicht nur vom Namen schöner, das zu trennen Es dürfte sogar kürzer und einfacher werden, wenn du es trennst.


Oh, und in Handler solltest du statt Dictionary Map nutzen

 

[mrBrown]

boolean exists(Object subscriber)
        {
            Object target = weakReference.get();

            return target == null ? false : target.equals(subscriber);
        }

Die würde ich tendenziell auch anders benennen - vllt sowas wie isSubscriber(Object subscriber)

 

[temi]

Und auch beim hinzufügen

Ups, korrigiert. Hätte ich das irgendwie testen können?
z.B. in der main:

foo = null;

Naja, die Methode macht halt immer noch mehrere Dinge - das wäre nicht nur vom Namen schöner, das zu trennen Es dürfte sogar kürzer und einfacher werden, wenn du es trennst.

Das ist schon richtig, aber:

for (Handler handler : toNotify)
        {
            handler.isAlive() ? handler.handle(event) : dead.add(handler);

//            if (!handler.handleAndIsAlive(event))
//            {
//                dead.add(handler);
//            }
        }

Ich weiß nicht so recht? Außerdem führt das jeweils in "isAlive" und in "handle" zu einem Aufruf von "weakReference.get()" Sagen wir mal "handleAndIsAlive" ist zumindest laufzeitfreundlicher.

Oh, und in Handler solltest du statt Dictionary Map nutzen

Warum?

 

[temi]

boolean exists(Object subscriber)
        {
            Object target = weakReference.get();

            return target == null ? false : target.equals(subscriber);
        }

Die würde ich tendenziell auch anders benennen - vllt sowas wie isSubscriber(Object subscriber)

boolean isSubscriberFor(Object subscriber)
        {
            Object target = weakReference.get();

            return target == null ? false : target.equals(subscriber);
        }

Das liest sich ganz angenehm, z.B.

public void remove(final Object subscriber)
    {
        handlers.removeIf(handler -> handler.isSubscriberFor(subscriber));
    }

 

[mrBrown]

Ups, korrigiert. Hätte ich das irgendwie testen können?
z.B. in der main:

Nur an der Stelle, auf der du drauf zu greifst

Ich weiß nicht so recht?

for (Handler handler : toNotify)
        {
            handler.handle(event);
        }
toNotify.removeIf(handler -> handler.isAlive()) ;)

Ganz ohne zusätzliche Liste
Außerdem lässt es sich so getrennt testen und nutzen.

Sagen wir mal "handleAndIsAlive" ist zumindest laufzeitfreundlicher.

Jein - so einfach ist das bei der JVM nicht

isAlive ist kurz genug, wird also sowieso inlined, der Overhead durch den Methodenaufruf fällt also weg.
Die anderen Methoden werden gleichzeitig kürzer, und sind damit eher Kandidat für inlining

Und durch die zusätzliche Liste, die du mit handleAndIsAlive für das entfernen brauchst, ist das langsamer als removeIf()

Warum?

This class is obsolete.

Map ist das Interface, was man stattdessen nutzen sollte.

 

[temi]

Nur an der Stelle, auf der du drauf zu greifst.

Ich meinte: Kann ich dafür sorgen, dass die Methode "isAlive()" false zurückgibt, weil keine Instanz mehr existiert? Dann hätte ich ja (in der ursprünglichen Version) eine NPE erhalten.

Ich habe grade mal versucht in der main foo zwischendrin auf null zu setzen, aber es funktioniert weiterhin alles wie bisher.

for (Handler handler : toNotify)
        {
            handler.handle(event);
        }
toNotify.removeIf(handler -> handler.isAlive()) ;)

Ganz ohne zusätzliche Liste
Außerdem lässt es sich so getrennt testen und nutzen.

Cool, aber wenn schon interne Iteration dann auch gleich so:

    public void publish(final Object event)
    {
        Objects.requireNonNull(event, "Event");

        ArrayList<Handler> toNotify = new ArrayList<>(handlers);

        toNotify.forEach(handler -> handler.handle(event));

        handlers.removeIf(handler -> !handler.isAlive());
    }

Stellt sich die Frage, ob ich dann toNotify überhaupt noch benötige? Das war nur notwendig wegen ConcurrentModificationException, wenn während des publishings ein handler entfernt wird (siehe Foo).

 

[mrBrown]

Ich meinte: Kann ich dafür sorgen, dass die Methode "isAlive()" false zurückgibt, weil keine Instanz mehr existiert? Dann hätte ich ja (in der ursprünglichen Version) eine NPE erhalten.

false zurückgeben, wenn die Instanz aufgeräumt wurde, ist doch genau das, was isAlive tun soll?
Ich versteh die Frage grad nicht?

Stellt sich die Frage, ob ich dann toNotify überhaupt noch benötige? Das war nur notwendig wegen ConcurrentModificationException, wenn während des publishings ein handler entfernt wird (siehe Foo).

Zumindest ist synchronisation nötig - die fehlt bisher noch ganz - und in Verbindung mit dieser sind Kopien der Liste oft hilfreich

 

[temi]

false zurückgeben, wenn die Instanz aufgeräumt wurde, ist doch genau das, was isAlive tun soll?
Ich versteh die Frage grad nicht?

Siehe Kommentar:

public static void main(String[] args)
    {
        IEventManager events = new EventManager();

        // inject EventManager and subscribe internally
        Foo foo = new Foo(events);
        Foo foe = new Foo(events);

        Bar bar = new Bar();
        events.subscribe(bar);
        
        bar = null; // Objekt ungültig machen???

        events.publish(new TextMessage("lorem ipsum")); // Ungültiger Handler sollte jetzt automatisch entfernt werden 
        events.publish(new CounterChanged(42));

        events.publish(new TextMessage("dolor sit amet")); // Event wird aber trotzdem noch von bar bearbeitet???
        events.publish(new CounterChanged(42));
    }

Ich möchte demnach ausprobieren, ob ein Handler mit ungültiger WeakReference korrekt entfernt wird. In der ursprünglichen Version hätte das noch zu einer NPE geführt.

 

[mrBrown]

Wirst du so nicht testen können

Die WeakReference wird nur dann geleert, wenn der GC läuft und das Objekt aufräumt, das ist irgendwann und im Zweifelsfall nie - z.B. in dem Fall nicht, weil die JVM alles aufm Stack alloziieren könnte.

 

[temi]

So hat es funktioniert:

bar = null;
System.gc();

 

[mrBrown]

So hat es funktioniert:

bar = null;
System.gc();

Für einen einmaligen Test mag das okay sein - das solltest du wen möglich nicht nutzen und auch nicht drauf vertrauen, dass es tut was du erwartest

 

[temi]

Zumindest ist synchronisation nötig - die fehlt bisher noch ganz - und in Verbindung mit dieser sind Kopien der Liste oft hilfreich

Mit Nebenläufigkeit habe ich mich noch nicht beschäftigt. Im Grunde müsste ich alle Zugriffe auf die List<Handler> handlers in einen synchronize Block setzen oder die Methode synchonisieren, oder?

Ist das dann ausreichend:

@Override
    public synchronized void subscribe(final Object subscriber)
    {
        Objects.requireNonNull(subscriber, "Subscriber");

        for (Handler handler : handlers)
        {
            if (handler.isSubscriberFor(subscriber))
            {
                throw new IllegalArgumentException("Subscriber already isSubscriberFor: '" + subscriber.getClass().getName() + "'");
            }
        }

        handlers.add(new Handler(subscriber));
    }

    @Override
    public synchronized void remove(final Object subscriber)
    {
        handlers.removeIf(handler -> handler.isSubscriberFor(subscriber));
    }

    @Override
    public void publish(final Object event)
    {
        Objects.requireNonNull(event, "Event");

        ArrayList<Handler> toNotify;

        synchronized (this)
        {
            handlers.removeIf(handler -> !handler.isAlive());

            toNotify = new ArrayList<>(handlers);
        }
       
        toNotify.forEach(handler -> handler.handle(event));
    }

 

[mrBrown]

Ja, sollte reichen.

Alternativ könnte man eine Synchronisierte Liste nutzen, dann muss man nicht selbst synchronisieren

 

[temi]

Aktueller Stand. Danke für die reichhaltigen und lehrreichen Informationen.

import java.lang.ref.WeakReference;
import java.lang.reflect.Method;
import java.lang.reflect.InvocationTargetException;
import java.util.Objects;
import java.util.List;
import java.util.ArrayList;
import java.util.Map;
import java.util.HashMap;


public final class EventManager implements IEventManager
{
    private static String DEFAULT_METHOD = "handle";
    private final String defaultMethod;

    private List<Handler> handlers = new ArrayList<>();

    public EventManager()
    {
        this(DEFAULT_METHOD);
    }

    public EventManager(final String defaultMethod)
    {
        if (defaultMethod == null || defaultMethod.trim().length() == 0)
        {
            throw new IllegalArgumentException("Default method must not be null or empty");
        }

        String removedSpaces = defaultMethod.replaceAll("\\s+","");

        this.defaultMethod = removedSpaces;
    }

    @Override
    public synchronized void subscribe(final Object subscriber)
    {
        Objects.requireNonNull(subscriber, "Subscriber");

        for (Handler handler : handlers)
        {
            if (handler.isSubscriberFor(subscriber))
            {
                throw new IllegalArgumentException("Subscriber is already registered: '" + subscriber.getClass().getName() + "'");
            }
        }

        handlers.add(new Handler(subscriber));
    }

    @Override
    public synchronized void remove(final Object subscriber)
    {
        Objects.requireNonNull(subscriber, "Subscriber");

        handlers.removeIf(h -> h.isSubscriberFor(subscriber));
    }

    @Override
    public void publish(final Object event)
    {
        Objects.requireNonNull(event, "Event");

        ArrayList<Handler> toNotify;

        synchronized (this)
        {
            handlers.removeIf(h -> !h.isAlive());

            toNotify = new ArrayList<>(handlers);
        }

        toNotify.forEach(h -> h.handle(event));
    }

    private class Handler
    {
        final WeakReference<Object> weakReference;
        final Map<Class, Method> supportedEvents = new HashMap<>();

        Handler(final Object handler)
        {
            weakReference = new WeakReference<>(handler);

            Method[] declaredMethods = handler.getClass().getDeclaredMethods();

            for (Method method : declaredMethods)
            {
                if (method.getName().equals(defaultMethod))
                {
                    Class[] parameterTypes = method.getParameterTypes();
                    supportedEvents.put(parameterTypes[0], method);
                }
            }

            if (supportedEvents.isEmpty())
                throw new IllegalArgumentException("Method '" + defaultMethod + "' is not implemented: '" + handler.getClass().getName() + "'");
        }

        void handle(final Object event)
        {
            Object target = weakReference.get();

            if (target == null) return;

            Method handler = supportedEvents.get(event.getClass());

            if (handler != null)
            {
                try
                {
                    handler.invoke(target, event);
                }

                catch (IllegalAccessException e)
                {
                    // todo: find a better way
                    System.err.println(e);
                }

                catch (InvocationTargetException e)
                {
                    // todo: find a better way
                    System.err.println(e);
                }
            }
        }

        boolean isSubscriberFor(final Object subscriber)
        {
            Object target = weakReference.get();

            return target != null && target.equals(subscriber);
        }

        boolean isAlive()
        {
            return weakReference.get() != null;
        }
    }
}