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

Sockets declared with Undertow @ServerEndpoint breaking when using quarkus.http.root-path #22053

Closed
mklueh opened this issue Dec 9, 2021 · 11 comments · Fixed by #25167
Closed
Labels
Milestone

Comments

@mklueh
Copy link
Contributor

mklueh commented Dec 9, 2021

Describe the bug

When using quarkus.http.root-path=/api/, the socket can't be connected to anymore, neither under the root path nor under the /api sub-route.

Using Quarkus 2.5.1

Expected behavior

To be working correctly under /api/ as well.

Actual behavior

This is the response when trying to connect with Postman

Could not connect to ws://localhost:8080/api
07:48:51
Error: Unexpected server response: 200
Handshake Details
Request URL: http://localhost:8080/api
Request Method: GET
Status Code: 200 OK
Request Headers
Sec-WebSocket-Version: 13
Sec-WebSocket-Key: h8vwu8Ok7TpH7785Q3Hn0g==
Connection: Upgrade
Upgrade: websocket
Sec-WebSocket-Extensions: permessage-deflate; client_max_window_bits
Host: localhost:8080
Response Headers
Content-Type: text/html
Last-Modified: Sun, 14 Nov 2021 08:05:42 GMT
Content-Length: 137
Accept-Ranges: bytes
Connection: close

image

@sandronm
Copy link

This bug remains unfixed with Quarkus 2.7.5.

@mklueh
Copy link
Contributor Author

mklueh commented Mar 22, 2022

@sandronm Glad I'm at least not the only one facing it

@sandersteenhoudt
Copy link

Oh man, I just spent a few hours scratching my head wondering what I was doing wrong while implementing websockets.. I kept getting a 404 no matter what I tried. This is quite annoying because now I need to change my other endpoints to accommodate this...

@gsmet
Copy link
Member

gsmet commented Apr 5, 2022

Could someone affected provide a small Maven reproducer for this issue?

@mklueh
Copy link
Contributor Author

mklueh commented Apr 5, 2022

@gsmet wouldn't it be enough to just add a test with the changed root path to the test suite? Because it's really just that, nothing else

@gsmet
Copy link
Member

gsmet commented Apr 5, 2022

Well, you can do that if you prefer. It can be a bit more work because of the size of the project.

@sandersteenhoudt
Copy link

@gsmet I'll see if I can get a reproducer together. No hard promises yet though, things are a bit hectic atm...

@mklueh
Copy link
Contributor Author

mklueh commented Apr 8, 2022

@gsmet will the reproducer be embedded into tests or is it just to show that this issue really happens?
I mean, I could also do it, but as I've said, it's really just that one line of properties + a minor websocket class like that one already existing in the docs:

https://quarkus.io/guides/websockets

package org.acme.websockets;

import java.util.Map;
import java.util.concurrent.ConcurrentHashMap;

import javax.enterprise.context.ApplicationScoped;
import javax.websocket.OnClose;
import javax.websocket.OnError;
import javax.websocket.OnMessage;
import javax.websocket.OnOpen;
import javax.websocket.server.PathParam;
import javax.websocket.server.ServerEndpoint;
import javax.websocket.Session;

@ServerEndpoint("/chat/{username}")         
@ApplicationScoped
public class ChatSocket {

    Map<String, Session> sessions = new ConcurrentHashMap<>(); 

    @OnOpen
    public void onOpen(Session session, @PathParam("username") String username) {
        sessions.put(username, session);
    }

    @OnClose
    public void onClose(Session session, @PathParam("username") String username) {
        sessions.remove(username);
        broadcast("User " + username + " left");
    }

    @OnError
    public void onError(Session session, @PathParam("username") String username, Throwable throwable) {
        sessions.remove(username);
        broadcast("User " + username + " left on error: " + throwable);
    }

    @OnMessage
    public void onMessage(String message, @PathParam("username") String username) {
        if (message.equalsIgnoreCase("_ready_")) {
            broadcast("User " + username + " joined");
        } else {
            broadcast(">> " + username + ": " + message);
        }
    }

    private void broadcast(String message) {
        sessions.values().forEach(s -> {
            s.getAsyncRemote().sendObject(message, result ->  {
                if (result.getException() != null) {
                    System.out.println("Unable to send message: " + result.getException());
                }
            });
        });
    }

}

Hard to imagine why there must be an extra project created just to show it

@gsmet
Copy link
Member

gsmet commented Apr 21, 2022

Hard to imagine why there must be an extra project created just to show it

Well, you're not juggling with many reports of people saying it's simple to reproduce and trying to reproduce it yourself. Having a self contained project with a test really makes our life easier and personally I prioritize issues that have one.

@stuartwdouglas I digged a bit this one and AFAICS we are not propagating the root path at all (or I missed it). This is pretty involved and I'm not sure where I should push the info in Quarkus HTTP. If you can point me to where we should handle things there, I can make sure this thing is propagated correctly to this place.

@stuartwdouglas
Copy link
Member

I will try and have a look sometime next week

stuartwdouglas added a commit to stuartwdouglas/quarkus that referenced this issue Apr 26, 2022
@stuartwdouglas
Copy link
Member

quarkusio/quarkus-http#103 should fix it, once it is merged a test is at #25156

@quarkus-bot quarkus-bot bot added this to the 2.9 - main milestone Apr 26, 2022
gsmet pushed a commit to stuartwdouglas/quarkus that referenced this issue Apr 26, 2022
holly-cummins pushed a commit to holly-cummins/quarkus that referenced this issue Apr 27, 2022
@gsmet gsmet modified the milestones: 2.9.0.CR1, 2.8.3.Final May 5, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants