Skip to content

[JENKINS-70103] WeakHashMap broken for looking up Jetty Session in WebSocket response #7412

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.

Sign up for GitHub

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

Merged
basil merged 4 commits into from
Nov 20, 2022
Merged

[JENKINS-70103] WeakHashMap broken for looking up Jetty Session in WebSocket response #7412

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
843d1b6
[JENKINS-70103] `WeakHashMap` broken for looking up Jetty `Session` i…
jglick Nov 18, 2022
f5a4617
Update core/src/main/java/jenkins/websocket/WebSockets.java
basil Nov 19, 2022
7d1c8b5
Update websocket/spi/src/main/java/jenkins/websocket/Provider.java
basil Nov 19, 2022
13cc83a
Merge branch 'master' into providerSession-JENKINS-70103
basil Nov 19, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 9 additions & 1 deletion core/src/main/java/jenkins/websocket/WebSockets.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -68,12 +68,20 @@ public static HttpResponse upgrade(WebSocketSession session) {
return (req, rsp, node) -> {
try {
session.handler = provider.handle(req, rsp, new Provider.Listener() {
private Object providerSession;

@Override
public void onWebSocketConnect() {
public void onWebSocketConnect(Object providerSession) {
this.providerSession = providerSession;
session.startPings();
session.opened();
}

@Override
public Object getProviderSession() {
return providerSession;
}

@Override
public void onWebSocketClose(int statusCode, String reason) {
session.stopPings();
Expand Down
11 changes: 2 additions & 9 deletions websocket/jetty10/src/main/java/jenkins/websocket/Jetty10Provider.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,9 +26,6 @@

import java.io.IOException;
import java.nio.ByteBuffer;
import java.util.Collections;
import java.util.Map;
import java.util.WeakHashMap;
import java.util.concurrent.CompletableFuture;
import java.util.concurrent.Future;
import javax.servlet.http.HttpServletRequest;
Expand All @@ -49,9 +46,6 @@ public class Jetty10Provider implements Provider {

private static final String ATTR_LISTENER = Jetty10Provider.class.getName() + ".listener";

// TODO does not seem possible to use HttpServletRequest.get/setAttribute for this
private static final Map<Listener, Session> sessions = Collections.synchronizedMap(new WeakHashMap<>());

public Jetty10Provider() {
JettyWebSocketServerContainer.class.hashCode();
}
Expand Down Expand Up @@ -101,7 +95,7 @@ public void close() throws IOException {
}

private Session session() {
Session session = sessions.get(listener);
Session session = (Session) listener.getProviderSession();
if (session == null) {
throw new IllegalStateException("missing session");
}
Expand Down Expand Up @@ -151,8 +145,7 @@ public void onWebSocketClose(int statusCode, String reason) {

@Override
public void onWebSocketConnect(Session session) {
sessions.put(listener, session);
listener.onWebSocketConnect();
listener.onWebSocketConnect(session);
}

@Override
Expand Down
11 changes: 2 additions & 9 deletions websocket/jetty9/src/main/java/jenkins/websocket/Jetty9Provider.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,9 +26,6 @@

import java.io.IOException;
import java.nio.ByteBuffer;
import java.util.Collections;
import java.util.Map;
import java.util.WeakHashMap;
import java.util.concurrent.Future;
import javax.servlet.http.HttpServletRequest;
import javax.servlet.http.HttpServletResponse;
Expand All @@ -48,9 +45,6 @@ public class Jetty9Provider implements Provider {

private static final String ATTR_LISTENER = Jetty9Provider.class.getName() + ".listener";

// TODO does not seem possible to use HttpServletRequest.get/setAttribute for this
private static final Map<Listener, Session> sessions = Collections.synchronizedMap(new WeakHashMap<>());

private WebSocketServletFactory factory;

public Jetty9Provider() {
Expand Down Expand Up @@ -104,7 +98,7 @@ public void close() throws IOException {
}

private Session session() {
Session session = sessions.get(listener);
Session session = (Session) listener.getProviderSession();
if (session == null) {
throw new IllegalStateException("missing session");
}
Expand Down Expand Up @@ -136,8 +130,7 @@ public void onWebSocketClose(int statusCode, String reason) {

@Override
public void onWebSocketConnect(Session session) {
sessions.put(listener, session);
listener.onWebSocketConnect();
listener.onWebSocketConnect(session);
}

@Override
Expand Down
4 changes: 3 additions & 1 deletion websocket/spi/src/main/java/jenkins/websocket/Provider.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,9 @@ interface Provider {

interface Listener {

void onWebSocketConnect();
void onWebSocketConnect(Object providerSession);

Object getProviderSession();

void onWebSocketClose(int statusCode, String reason);

Expand Down