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

Document limitation around the port of localAddress in StandardWebSocketSession #34304

Closed
Spikhalskiy opened this issue Jan 22, 2025 · 5 comments
Assignees
Labels
in: web Issues in web modules (web, webmvc, webflux, websocket) type: documentation A documentation task
Milestone

Comments

@Spikhalskiy
Copy link

Spikhalskiy commented Jan 22, 2025

During migration from Spring 6.0 and replacing JettyWebSocketClient with StandardWebSocketClient we noticed that StandardWebSocketSession incorrectly returns a remote port instead of a local port from WebSocketSession#getLocalAddress() for the client WS session. Old removed JettyWebSocketSession was implementing this method correctly. It was returning the port of the local socket of the underlying TCP connection.

This problem comes from the following implementation in StandardWebsocketClient which is obviously erroneous:

	@Override
	protected CompletableFuture<WebSocketSession> executeInternal(WebSocketHandler webSocketHandler,
			HttpHeaders headers, final URI uri, List<String> protocols,
			List<WebSocketExtension> extensions, Map<String, Object> attributes) {

		int port = getPort(uri);
		InetSocketAddress localAddress = new InetSocketAddress(getLocalHost(), port);
		InetSocketAddress remoteAddress = new InetSocketAddress(uri.getHost(), port);

		StandardWebSocketSession session = new StandardWebSocketSession(headers,
				attributes, localAddress, remoteAddress);

Client WS Session shouldn't use the same port value for both the local and remote addresses.

There was a previous report about this bug and regression that was incorrectly closed for being a question: #22395

Workaround

To work around this bug, instead of

WebSocketSession session = ...
InetSocketAddress localAddress = session.getLocalAddress();

users should do

InetSocketAddress localAddress = (InetSocketAddress)((JakartaWebSocketSession)(((StandardWebSocketSession)
        session).getNativeSession())).getCoreSession().getLocalAddress();

when are using Jakarta WS implementation.

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged or decided on label Jan 22, 2025
@Spikhalskiy Spikhalskiy changed the title StandardWebSocketClient returns incorrect LocalAddress, regression from JettyWebSocketClient StandardWebSocketSession returns incorrect LocalAddress, regression from JettyWebSocketSession Jan 22, 2025
@sbrannen sbrannen added the in: web Issues in web modules (web, webmvc, webflux, websocket) label Jan 23, 2025
@sbrannen sbrannen changed the title StandardWebSocketSession returns incorrect LocalAddress, regression from JettyWebSocketSession StandardWebSocketClient creates WebSocketSession with local address with incorrect port Jan 23, 2025
@rstoyanchev
Copy link
Contributor

Thanks for the report. You're correct, but jakarta.session.Session doesn't make this available and there is no other way to get it. I think we can update the Javadoc to warn about it.

@Spikhalskiy
Copy link
Author

Spikhalskiy commented Jan 23, 2025

@rstoyanchev Do you think it would be productive to start a conversation in the Jakarta Websocket API project about adding a #getLocalAddress() method to a session? It's not intuitive why this information should be encapsulated and not be exposed in the session.
I think it's reasonable to expect an API to allow retrieving a local port the session is bound to.

@rstoyanchev
Copy link
Contributor

I think it is a good idea. I found jakartaee/websocket#255 for remote address that mentions local address, but it was created a while ago. Maybe it can be revived with more comments and votes. Perhaps the title can be updated too to reflect a request for both local and remote address.

@rstoyanchev rstoyanchev self-assigned this Jan 23, 2025
@rstoyanchev rstoyanchev added type: documentation A documentation task and removed status: waiting-for-triage An issue we've not yet triaged or decided on labels Jan 23, 2025
@rstoyanchev rstoyanchev added this to the 6.2.3 milestone Jan 23, 2025
@rstoyanchev rstoyanchev changed the title StandardWebSocketClient creates WebSocketSession with local address with incorrect port Document limitation around the port of localAddress in StandardWebSocketSession Jan 23, 2025
@Spikhalskiy
Copy link
Author

Spikhalskiy commented Jan 27, 2025

@rstoyanchev I will open a discussion in https://github.com/jakartaee/websocket referencing this issue sometime later.

But don't you think that it's a better solution to make StandardWebSocketSession.getLocalAddress throw in case no correct values can be determined? We just faced a pretty unpleasant long investigation because we were using getLocalAddress as a key to maintain an internal structure of open WS clients and the silent change of the method behavior led to a problem that wasn't trivial to trace. If this method was throwing, it would be immediately clear what's going on.

@rstoyanchev
Copy link
Contributor

rstoyanchev commented Jan 27, 2025

The method allows for returning null. We could probably do that rather than throw an exception. I've created #34331.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
in: web Issues in web modules (web, webmvc, webflux, websocket) type: documentation A documentation task
Projects
None yet
Development

No branches or pull requests

4 participants