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

config.enableSimpleBroker("/topic", "/queue"); Should be config.enableSimpleBroker("/topic", "queue"); [SPR-16275] #20822

Closed
spring-issuemaster opened this issue Dec 7, 2017 · 11 comments

Comments

Projects
None yet
2 participants
@spring-issuemaster
Copy link
Collaborator

commented Dec 7, 2017

Aray Chou opened SPR-16275 and commented

Hello, there is a error in WebSocket document, https://docs.spring.io/spring/docs/4.3.x/spring-framework-reference/html/websocket.html
the Doc says: I shoud config Borker as :
config.enableSimpleBroker("/topic", "/queue");

but if I conifg as above, the following code won't work:
template.convertAndSendToUser("Aray","/queue/trade",new Greeting("hhhh"));

The working config should be :
config.enableSimpleBroker("/topic", "queue"); (it is "queue" NOT "/queue")

I debuged and found this: the message send by template.convertAndSendToUser, its destination will finnally be changed into a string like this: "queue/trade-users4pkn4bg", it doesn't start with "/queue".
and the method org.springframework.messaging.simp.broker.AbstractBrokerMessageHandler#checkDestinationPrefix will return *false *if we config "/queue" as the Doc says.

I am not sure, we should change the Doc or change the Java Codes.

thanks


Affects: 4.3.13

Reference URL: https://docs.spring.io/spring/docs/4.3.x/spring-framework-reference/html/websocket.html

Issue Links:

  • #18616 DefaultUserDestinationResolver does not support "." separator convention

Backported to: 4.3.14

@spring-issuemaster

This comment has been minimized.

Copy link
Collaborator Author

commented Dec 7, 2017

Aray Chou commented

template.convertAndSendToUser with both StompBrokerRelay and SimpleBroker are buggy in version 4.3.13

I found a working example, which use 5.0.2 instead of 4.3.13, https://github.com/rstoyanchev/spring-websocket-portfolio.git.

@spring-issuemaster

This comment has been minimized.

Copy link
Collaborator Author

commented Dec 11, 2017

Rossen Stoyanchev commented

The documentation is correct. That's not the issue. The spring-websocket-portfolio sample works fine with 4.3.13 too.

What the SimpMessagingTemplate does is simply to insert the user destination prefix ("/user/" by default). You can see it here and you can see a test that demonstrates the outcome in the 4.3.x branch. Such a user destination prefixed message is then processed by the UserDestinationMessageHandler which transforms the destination into something like "/queue/trade-users4pkn4bg" and sends it again for further processing (by the broker).

You need to put a breakpoint in DefaultUserDestinationResolver to see why the transformation of the destination doesn't come out as expected. I could not reproduce it myself. The only thing that comes to mind is if you've configured your PathMatcher to use another separator such as "." (dot) instead of "/" in which case this is expected behavior.

@spring-issuemaster

This comment has been minimized.

Copy link
Collaborator Author

commented Dec 12, 2017

Aray Chou commented

Thanks Rossen, I do use "." as separator. I will create a demo to reproduce this issue latter.

@spring-issuemaster

This comment has been minimized.

Copy link
Collaborator Author

commented Dec 12, 2017

Aray Chou commented

Hi Rossen, I submit a test code to: https://github.com/ArayChou/gs-messaging-stomp-websocket-test.
I changed app.js and hello.WebSocketConfig, add a class hello.MessageSendService to call template.convertAndSendToUser

Yes, as you metioned: Only if I use "." as separator, It does NOT work.

I am try to use it with rabbitMQ, so I need "." as separator.

How can I overcome this, thank you very much!

@spring-issuemaster

This comment has been minimized.

Copy link
Collaborator Author

commented Dec 12, 2017

Rossen Stoyanchev commented

Okay so this is expected behavior then, no need to provide a demo. When you switch to using "." as separator, based on #18616 (in 4.3) we refined the behavior to leave out the leading slash. The documentation says "/queue" which is accurate based on default configuration with "/" as separator. Or did you mean then that we should change the documentation in the section "Dot as Separator"?

@spring-issuemaster

This comment has been minimized.

Copy link
Collaborator Author

commented Dec 12, 2017

Aray Chou commented

Hi Rossen, I still don't know how to make template.convertAndSendToUser("Aray","/queue/trade",new Greeting("hhhh")); work. I will use rabbitMQ at product environment. So I have to use "." as separator.

I think other guys may have the same problem, It is better to indicate this scenario and how to solve it in the Doc. Thanks!

@spring-issuemaster

This comment has been minimized.

Copy link
Collaborator Author

commented Dec 12, 2017

Rossen Stoyanchev commented

You had me confused since originally you described this as a documentation issue. You're actually saying that changing from "/queue" to "queue" does not make it work? Can you please provide a fresh description and a demo project. In other words don't talk about how the documentation should change, but what you tried and what happened.

Also please elaborate why you say you must use "." as separator? The spring-websocket-portfolio for example works fine without switching to using "." and it works with RabbitMQ.

@spring-issuemaster

This comment has been minimized.

Copy link
Collaborator Author

commented Dec 13, 2017

Aray Chou commented

Thank you very much for your patient, Rossen. I am new to spring-websocket, I want to use “.” as separator since I need enable rabbitMQ in the product environment.

I changed the demo project in the Getting Started document, and submit it to this place: https://github.com/ArayChou/gs-messaging-stomp-websocket-test . please kindly check its master brunch, thanks.

I send message to user in hello.MessageSendService#run via:

template.convertAndSendToUser("Aray", "/queue/trade", new Greeting("Message sent by convertAndSendToUser. i=" + i++));

I subscribe “/user/queue/trade” in app.js like this:

stompClient.subscribe('/user/queue/trade', function (greeting) {
    console.log(greeting.body);
    showGreeting(JSON.parse(greeting.body).content);
});

I demonstrate user login in hello.WebSocketConfig#registerStompEndpoints like this: (all connection will login as Aray)

registry.addEndpoint("/gs-guide-websocket")
        .setHandshakeHandler(new DefaultHandshakeHandler() {
            @Override
            protected Principal determineUser(ServerHttpRequest request, WebSocketHandler wsHandler,
                                              Map<String, Object> attributes) {
                if (true) {
                    return new Principal() {
                        @Override
                        public String getName() {
                            return "Aray";
                        }

                        @Override
                        public boolean implies(Subject subject) {
                            return false;
                        }
                    };
                } else
                    throw new HandshakeFailureException("Not Logined");
            }
        })

Please kindly let me know how to make this work. I am really confused by the Doc. Because the example code is actually mixed with “/” and “.”

I think If I make the simpleBroker works, and I should make the StomeBrokerRelay with rabbitMQ works.
Thanks very much.

@spring-issuemaster

This comment has been minimized.

Copy link
Collaborator Author

commented Jan 9, 2018

Rossen Stoyanchev commented

Sorry for the delay. Yes I do see the issue now. This can be traced back to a 4.3 change (#18616) after which the documentation should have been updated as well. I will experiment a little more, make the necessary changes, and will then update this ticket.

@spring-issuemaster

This comment has been minimized.

Copy link
Collaborator Author

commented Jan 19, 2018

Rossen Stoyanchev commented

I've gone back and forth on this for which I apologize but I think I've got it now.

The use of "." as separator was originally designed for annotated controllers where an AntPathMatcher is used to route to @MessageMapping methods. By contrast, to route to the broker only takes a simple destination prefix check, so the separator shouldn't matter. Furthermore RabbitMQ uses "/" for STOMP destination prefixes, e.g. /queue/<name>, /topic/<name>, which is why the docs show "/topic" and "/queue" and that actually worked as advertised until 4.3.

#18616 came along to show that with some brokers (Artemis) when translating from a user destination we need to know if the target queue name should have a leading slash or not because in Artemis destinations don't use "/" at all. So we fixed that issue but overlooked the fact that in RabbitMQ destination prefixes use "/".

To be clear it is not necessary to configure "." as separator in order to use RabbitMQ. Again the separator config option is mainly for annotated controllers (and for that it's mostly a preference). At the same time we do have to be careful to make sure the use of "." in annotated controller patterns doesn't get in the way of routing messages to the broker so I'm going to make a follow-up improvement to #18616 to change how the DefaultUserDestinationResolver decides whether to remove or keep the leading slash and to also make the docs more complete on the topic.

In summary:

  • the "." vs "/" separator config option is for controllers; it shouldn't matter for routing to the broker where we only check prefixes.
  • "/topic" and "/queue" is the right config for RabbitMQ; after the fix for #18616, when using "." messages for the broker are not correctly routed.
  • "topic." and "queue." is the right config for Artemis; #18616 ensured that when using "." messages for the broker are correctly routed, although arguably the use of "." shouldn't be required.
@spring-issuemaster

This comment has been minimized.

Copy link
Collaborator Author

commented Jan 20, 2018

Rossen Stoyanchev commented

This should be fixed now. The updated documentation and added tests.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.