Skip to content

Commit

Permalink
[bidi] [java] Ensure the listeners returns an id (#14215)
Browse files Browse the repository at this point in the history
  • Loading branch information
pujagani authored Jul 1, 2024
1 parent 5494108 commit 48fd956
Show file tree
Hide file tree
Showing 3 changed files with 46 additions and 16 deletions.
12 changes: 8 additions & 4 deletions java/src/org/openqa/selenium/bidi/BiDi.java
Original file line number Diff line number Diff line change
Expand Up @@ -51,15 +51,15 @@ public <X> X send(Command<X> command) {
return connection.sendAndWait(command, timeout);
}

public <X> void addListener(Event<X> event, Consumer<X> handler) {
public <X> long addListener(Event<X> event, Consumer<X> handler) {
Require.nonNull("Event to listen for", event);
Require.nonNull("Handler to call", handler);

send(
new Command<>(
"session.subscribe", Map.of("events", Collections.singletonList(event.getMethod()))));

connection.addListener(event, handler);
return connection.addListener(event, handler);
}

public <X> void addListener(String browsingContextId, Event<X> event, Consumer<X> handler) {
Expand All @@ -79,7 +79,7 @@ public <X> void addListener(String browsingContextId, Event<X> event, Consumer<X
connection.addListener(event, handler);
}

public <X> void addListener(Set<String> browsingContextIds, Event<X> event, Consumer<X> handler) {
public <X> long addListener(Set<String> browsingContextIds, Event<X> event, Consumer<X> handler) {
Require.nonNull("List of browsing context ids", browsingContextIds);
Require.nonNull("Event to listen for", event);
Require.nonNull("Handler to call", handler);
Expand All @@ -93,7 +93,7 @@ public <X> void addListener(Set<String> browsingContextIds, Event<X> event, Cons
"events",
Collections.singletonList(event.getMethod()))));

connection.addListener(event, handler);
return connection.addListener(event, handler);
}

public <X> void clearListener(Event<X> event) {
Expand All @@ -111,6 +111,10 @@ public <X> void clearListener(Event<X> event) {
}
}

public void removeListener(long id) {
connection.removeListener(id);
}

public void clearListeners() {
connection.clearListeners();
}
Expand Down
36 changes: 31 additions & 5 deletions java/src/org/openqa/selenium/bidi/Connection.java
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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<Long, Consumer<Either<Throwable, JsonInput>>> methodCallbacks =
new ConcurrentHashMap<>();
private final ReadWriteLock callbacksLock = new ReentrantReadWriteLock(true);
private final Map<Event<?>, List<Consumer<?>>> eventCallbacks = new HashMap<>();
private final Map<Event<?>, Map<Long, Consumer<?>>> eventCallbacks = new HashMap<>();
private final HttpClient client;
private final AtomicBoolean underlyingSocketClosed = new AtomicBoolean();

Expand Down Expand Up @@ -180,17 +180,26 @@ public <X> X sendAndWait(Command<X> command, Duration timeout) {
}
}

public <X> void addListener(Event<X> event, Consumer<X> handler) {
public <X> long addListener(Event<X> event, Consumer<X> 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<Long, Consumer<?>> map = new HashMap<>();
map.put(id, handler);
return map;
});

This comment has been minimized.

Copy link
@joerg1985

joerg1985 Jul 2, 2024

Member

@pujagani this will only add the first handler to the map.

This comment has been minimized.

Copy link
@pujagani

pujagani Jul 2, 2024

Author Contributor

I was actually work on break one change in two PRs and didn't realize that things got messy. Let me fix these things first. Thank you!

} finally {
lock.unlock();
}
return id;
}

public <X> void clearListener(Event<X> event) {
Expand All @@ -203,6 +212,23 @@ public <X> void clearListener(Event<X> event) {
}
}

public void removeListener(long id) {
Lock lock = callbacksLock.writeLock();
lock.lock();
try {
eventCallbacks.forEach((k, v) -> v.remove(id));
eventCallbacks.forEach(
(k, v) -> {
v.remove(id);
if (v.isEmpty()) {
eventCallbacks.remove(k);
}
});

This comment has been minimized.

Copy link
@joerg1985

joerg1985 Jul 2, 2024

Member

@pujagani This looks broken too, the list is itterated twice and in general it is a bad idea to remove while iterating.
How about: eventCallbacks.values().removeIf((v) -> v.remove(id) && v.isEmpty());

This comment has been minimized.

Copy link
@pujagani

pujagani Jul 2, 2024

Author Contributor

This I realized, the next PR in works fixes it.

} finally {
lock.unlock();
}
}

public <X> boolean isEventSubscribed(Event<X> event) {
Lock lock = callbacksLock.writeLock();
lock.lock();
Expand Down Expand Up @@ -354,7 +380,7 @@ private void handleEventResponse(Map<String, Object> rawDataMap) {

final Object finalValue = value;

for (Consumer<?> action : event.getValue()) {
for (Consumer<?> action : event.getValue().values()) {
@SuppressWarnings("unchecked")
Consumer<Object> obj = (Consumer<Object>) action;
LOG.log(
Expand Down
14 changes: 7 additions & 7 deletions java/src/org/openqa/selenium/bidi/module/LogInspector.java
Original file line number Diff line number Diff line change
Expand Up @@ -61,11 +61,11 @@ public LogInspector(Set<String> browsingContextIds, WebDriver driver) {
this.browsingContextIds = browsingContextIds;
}

public void onConsoleEntry(Consumer<ConsoleLogEntry> consumer) {
public long onConsoleEntry(Consumer<ConsoleLogEntry> consumer) {
Consumer<LogEntry> logEntryConsumer =
logEntry -> logEntry.getConsoleLogEntry().ifPresent(consumer);

addLogEntryAddedListener(logEntryConsumer);
return addLogEntryAddedListener(logEntryConsumer);
}

public void onConsoleEntry(Consumer<ConsoleLogEntry> consumer, FilterBy filter) {
Expand Down Expand Up @@ -105,7 +105,7 @@ public void onJavaScriptLog(Consumer<JavascriptLogEntry> consumer, FilterBy filt
addLogEntryAddedListener(logEntryConsumer);
}

public void onJavaScriptException(Consumer<JavascriptLogEntry> consumer) {
public long onJavaScriptException(Consumer<JavascriptLogEntry> consumer) {
Consumer<LogEntry> logEntryConsumer =
logEntry ->
logEntry
Expand All @@ -117,7 +117,7 @@ public void onJavaScriptException(Consumer<JavascriptLogEntry> consumer) {
}
});

addLogEntryAddedListener(logEntryConsumer);
return addLogEntryAddedListener(logEntryConsumer);
}

public void onGenericLog(Consumer<GenericLogEntry> consumer) {
Expand Down Expand Up @@ -163,11 +163,11 @@ public void onLog(Consumer<LogEntry> consumer, FilterBy filter) {
addLogEntryAddedListener(logEntryConsumer);
}

private void addLogEntryAddedListener(Consumer<LogEntry> consumer) {
private long addLogEntryAddedListener(Consumer<LogEntry> 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);
}
}

Expand Down

0 comments on commit 48fd956

Please sign in to comment.