From 05b6b89d3dd1119fe7b28a7072d7d49846ca53a4 Mon Sep 17 00:00:00 2001 From: Puja Jagani Date: Mon, 1 Jul 2024 16:34:47 +0530 Subject: [PATCH 1/2] [bidi] [java] Ensure the listeners returns an id This is the base for implementing the high-level BiDi API --- java/src/org/openqa/selenium/bidi/BiDi.java | 12 +++++--- .../org/openqa/selenium/bidi/Connection.java | 29 +++++++++++++++---- .../selenium/bidi/module/LogInspector.java | 14 ++++----- 3 files changed, 39 insertions(+), 16 deletions(-) diff --git a/java/src/org/openqa/selenium/bidi/BiDi.java b/java/src/org/openqa/selenium/bidi/BiDi.java index 100b45404bb84..912355a75a61c 100644 --- a/java/src/org/openqa/selenium/bidi/BiDi.java +++ b/java/src/org/openqa/selenium/bidi/BiDi.java @@ -51,7 +51,7 @@ public X send(Command command) { return connection.sendAndWait(command, timeout); } - public void addListener(Event event, Consumer handler) { + public long addListener(Event event, Consumer handler) { Require.nonNull("Event to listen for", event); Require.nonNull("Handler to call", handler); @@ -59,7 +59,7 @@ public void addListener(Event event, Consumer handler) { new Command<>( "session.subscribe", Map.of("events", Collections.singletonList(event.getMethod())))); - connection.addListener(event, handler); + return connection.addListener(event, handler); } public void addListener(String browsingContextId, Event event, Consumer handler) { @@ -79,7 +79,7 @@ public void addListener(String browsingContextId, Event event, Consumer void addListener(Set browsingContextIds, Event event, Consumer handler) { + public long addListener(Set browsingContextIds, Event event, Consumer handler) { Require.nonNull("List of browsing context ids", browsingContextIds); Require.nonNull("Event to listen for", event); Require.nonNull("Handler to call", handler); @@ -93,7 +93,7 @@ public void addListener(Set browsingContextIds, Event event, Cons "events", Collections.singletonList(event.getMethod())))); - connection.addListener(event, handler); + return connection.addListener(event, handler); } public void clearListener(Event event) { @@ -111,6 +111,10 @@ public void clearListener(Event event) { } } + public void removeListener(long id) { + connection.removeListener(id); + } + public void clearListeners() { connection.clearListeners(); } diff --git a/java/src/org/openqa/selenium/bidi/Connection.java b/java/src/org/openqa/selenium/bidi/Connection.java index e7827b80f8421..a56f27edb3b62 100644 --- a/java/src/org/openqa/selenium/bidi/Connection.java +++ b/java/src/org/openqa/selenium/bidi/Connection.java @@ -25,7 +25,6 @@ import java.io.Closeable; import java.io.StringReader; import java.time.Duration; -import java.util.ArrayList; import java.util.HashMap; import java.util.List; import java.util.Map; @@ -66,11 +65,12 @@ public class Connection implements Closeable { return thread; }); private static final AtomicLong NEXT_ID = new AtomicLong(1L); + private final AtomicLong EVENT_CALLBACK_ID = new AtomicLong(1); private WebSocket socket; private final Map>> methodCallbacks = new ConcurrentHashMap<>(); private final ReadWriteLock callbacksLock = new ReentrantReadWriteLock(true); - private final Map, List>> eventCallbacks = new HashMap<>(); + private final Map, Map>> eventCallbacks = new HashMap<>(); private final HttpClient client; private final AtomicBoolean underlyingSocketClosed = new AtomicBoolean(); @@ -180,17 +180,26 @@ public X sendAndWait(Command command, Duration timeout) { } } - public void addListener(Event event, Consumer handler) { + public long addListener(Event event, Consumer handler) { Require.nonNull("Event to listen for", event); Require.nonNull("Handler to call", handler); + long id = EVENT_CALLBACK_ID.getAndIncrement(); + Lock lock = callbacksLock.writeLock(); lock.lock(); try { - eventCallbacks.computeIfAbsent(event, (key) -> new ArrayList<>()).add(handler); + eventCallbacks.computeIfAbsent( + event, + key -> { + HashMap> map = new HashMap<>(); + map.put(id, handler); + return map; + }); } finally { lock.unlock(); } + return id; } public void clearListener(Event event) { @@ -203,6 +212,16 @@ public void clearListener(Event event) { } } + public void removeListener(long id) { + Lock lock = callbacksLock.writeLock(); + lock.lock(); + try { + eventCallbacks.forEach((k, v) -> v.remove(id)); + } finally { + lock.unlock(); + } + } + public boolean isEventSubscribed(Event event) { Lock lock = callbacksLock.writeLock(); lock.lock(); @@ -354,7 +373,7 @@ private void handleEventResponse(Map rawDataMap) { final Object finalValue = value; - for (Consumer action : event.getValue()) { + for (Consumer action : event.getValue().values()) { @SuppressWarnings("unchecked") Consumer obj = (Consumer) action; LOG.log( diff --git a/java/src/org/openqa/selenium/bidi/module/LogInspector.java b/java/src/org/openqa/selenium/bidi/module/LogInspector.java index fde35d6b850ba..27c2adae27452 100644 --- a/java/src/org/openqa/selenium/bidi/module/LogInspector.java +++ b/java/src/org/openqa/selenium/bidi/module/LogInspector.java @@ -61,11 +61,11 @@ public LogInspector(Set browsingContextIds, WebDriver driver) { this.browsingContextIds = browsingContextIds; } - public void onConsoleEntry(Consumer consumer) { + public long onConsoleEntry(Consumer consumer) { Consumer logEntryConsumer = logEntry -> logEntry.getConsoleLogEntry().ifPresent(consumer); - addLogEntryAddedListener(logEntryConsumer); + return addLogEntryAddedListener(logEntryConsumer); } public void onConsoleEntry(Consumer consumer, FilterBy filter) { @@ -105,7 +105,7 @@ public void onJavaScriptLog(Consumer consumer, FilterBy filt addLogEntryAddedListener(logEntryConsumer); } - public void onJavaScriptException(Consumer consumer) { + public long onJavaScriptException(Consumer consumer) { Consumer logEntryConsumer = logEntry -> logEntry @@ -117,7 +117,7 @@ public void onJavaScriptException(Consumer consumer) { } }); - addLogEntryAddedListener(logEntryConsumer); + return addLogEntryAddedListener(logEntryConsumer); } public void onGenericLog(Consumer consumer) { @@ -163,11 +163,11 @@ public void onLog(Consumer consumer, FilterBy filter) { addLogEntryAddedListener(logEntryConsumer); } - private void addLogEntryAddedListener(Consumer consumer) { + private long addLogEntryAddedListener(Consumer consumer) { if (browsingContextIds.isEmpty()) { - this.bidi.addListener(Log.entryAdded(), consumer); + return this.bidi.addListener(Log.entryAdded(), consumer); } else { - this.bidi.addListener(browsingContextIds, Log.entryAdded(), consumer); + return this.bidi.addListener(browsingContextIds, Log.entryAdded(), consumer); } } From 338ffddc8923374ff8dfe0651364221102577eaf Mon Sep 17 00:00:00 2001 From: Puja Jagani Date: Mon, 1 Jul 2024 16:46:30 +0530 Subject: [PATCH 2/2] [java] Ensure proper cleanup while removing handlers --- java/src/org/openqa/selenium/bidi/Connection.java | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/java/src/org/openqa/selenium/bidi/Connection.java b/java/src/org/openqa/selenium/bidi/Connection.java index a56f27edb3b62..dd6bce851fb22 100644 --- a/java/src/org/openqa/selenium/bidi/Connection.java +++ b/java/src/org/openqa/selenium/bidi/Connection.java @@ -217,6 +217,13 @@ public void removeListener(long id) { lock.lock(); try { eventCallbacks.forEach((k, v) -> v.remove(id)); + eventCallbacks.forEach( + (k, v) -> { + v.remove(id); + if (v.isEmpty()) { + eventCallbacks.remove(k); + } + }); } finally { lock.unlock(); }