JSteunou / webstomp-client

Stomp client over websocket for browsers
Apache License 2.0
299 stars 59 forks source link

Deprecation Warning #75

Closed krusche closed 5 years ago

krusche commented 6 years ago

We are using SockJS as default websocket type:

const socket = new SockJS(url);
const stompClient = Stomp.over(socket);

Unfortunately this always leads to the deprecation warning

DEPRECATED: is not a recognized STOMP version. In next major client version, this will close the connection

because 'protocol' is an empty string in SockJS. I can't find a way to specify protocol in such a case.

Could you please deactivate the deprecation warning in such a case?

Marcsimm commented 5 years ago

We are getting the same problem. We are using native WebSocket and enable stomp over it

const ws = new WebSocket(SOCKETS_HOST)

const client = webstomp.over(ws, { debug: false })

DEPRECATED: is not a recognized STOMP version. In next major client version, this will close the connection

our sentry is bomabred with these errors.

JSteunou commented 5 years ago

Cant you just set correct stomp version?

This deprecated warning is here for a reason, I planned to be more strict about this in the future and throw an error if there is a stomp version mismatch

krusche commented 5 years ago

I tried all possible ways, but the protocol attribute is readonly and cannot be specified in the constructor, so no, it is not possible. We could create an issue on the SockJS project and hope they will allow it.

If you really throw an error, it will be impossible for us to use a new release. Would it be an option that you only log a warning in development mode?

Marcsimm commented 5 years ago

We updated to the latest version 1.2.4 and tested but we are still getting the same problem

DEPRECATED: is not a recognized STOMP version. In next major client version, this will close the connection

JSteunou commented 5 years ago

@Marcsimm I did not remove the warning nor published a new version about it, please read my comment above

@krusche I see. A PR into SockJS would be a good thing, I'm surprised that headers cannot be set

Polve commented 5 years ago

Even if SockJS could allow headers, what about running it from node, using the ws npm?

krusche commented 5 years ago

@JSteunou At the moment, you can only specify the URL and 3 options (server, sessionId and transport) in the TypeScript constructor that we use. I am not a JS expert though, so there might be an option that I am not aware of.

Please have a look into https://github.com/sockjs/sockjs-client/blob/master/lib/main.js in line 32 to 43.

Here we have function SockJS(url, protocols, options) { where protocols seems to be something different and then later this.protocol = '';

The code with the deprecation checks for protocol which is empty in my case, so I get the warning.

JSteunou commented 5 years ago

I would like to not take this lightly because as you can see here https://en.wikipedia.org/wiki/WebSocket#Protocol_handshake WS protocols, seen as the header Sec-WebSocket-Protocol is the way the client can say I speak stompv1 or stompv2 and the server say okay lets talk stompv2 then the client adapt itself to the server choice.

We are lucky STOMP did not introduce a lot of feature nor breaking changes, because having a forced handshake with a default protocol or empty protocol can lead to incomprehension between client and server.

All WS server & client should handle WS protocol headers

krusche commented 5 years ago

@JSteunou I can understand your point and support your opinion that webstomp-client should follow the standard. I would be glad to tell SockJS which protocol we want to use. Do you see any option how to include this into SockJS?

JSteunou commented 5 years ago

I'm not a SockJS expert but I will try to look at it when having some bandwidth for it

Marcsimm commented 5 years ago

Is there at least a way to hide the warning so sentry won't bombard us with errors

DEPRECATED: is not a recognized STOMP version. In next major client version, this will close the connection.

JSteunou commented 5 years ago

Could you try https://github.com/JSteunou/webstomp-client/tree/hotfix/protocol

I put several fallbacks on protocol

  1. if there are no ws.protocol (being server or ws client fault) I will fallback on Client options.protocols 1st item
  2. event with this, if there is still a no match in supported version, I kept the 2nd fallback, but change it from a list of all STOMP versions to only the newest. If you know you have issues with your server or your ws client, and need STOMP 1.0 instead of 1.2, force the protocol with options.protocols

And for a better logging @Marcsimm I remove the console.warn to use the internal debug function which does not log if options.debug is falsy.

JSteunou commented 5 years ago

Details on PR https://github.com/JSteunou/webstomp-client/pull/76

Polve commented 5 years ago

The hotfix branch is not yet working for me if I don't set procols option, i.e. if I use this:

const options = { debug: false, heartbeat: false}

the workaround is still to do:

const options = { debug: false, heartbeat: false, protocols: ['v12.stomp'] }

Polve commented 5 years ago

This is a capture of the Network tab: screenshot_04_172127

Polve commented 5 years ago

Request Headers:

Host: api-test.melis.io User-Agent: Mozilla/5.0 (X11; Ubuntu; Linux x86_64; rv:62.0) Gecko/20100101 Firefox/62.0 Accept: text/html,application/xhtml+xml,application/xml;q=0.9,/;q=0.8 Accept-Language: en,it;q=0.8,es;q=0.6,fr;q=0.4,en-US;q=0.2 Accept-Encoding: gzip, deflate, br Sec-WebSocket-Version: 13 Origin: http://localhost:4400 Sec-WebSocket-Protocol: v10.stomp, v11.stomp, v12.stomp Sec-WebSocket-Extensions: permessage-deflate Sec-WebSocket-Key: hzCOKVYPBSC3vOxPK/R7wg== Cookie: __cfduid=db807016189fb636550803c19a91459241529858756 DNT: 1 Connection: keep-alive, Upgrade Pragma: no-cache Cache-Control: no-cache Upgrade: websocket

Response Headers:

HTTP/1.1 101 Date: Thu, 04 Oct 2018 15:20:18 GMT Connection: upgrade Upgrade: websocket Sec-WebSocket-Accept: NgVB9sZVgas3+/UJK1Vke+Cpysw= Sec-WebSocket-Protocol: v10.stomp Sec-WebSocket-Extensions: permessage-deflate X-Content-Type-Options: nosniff X-XSS-Protection: 1; mode=block Cache-Control: no-cache, no-store, max-age=0, must-revalidate Pragma: no-cache Expires: 0 Strict-Transport-Security: max-age=0; includeSubDomains; preload X-Frame-Options: DENY Expect-CT: max-age=604800, report-uri="https://report-uri.cloudflare.com/cdn-cgi/beacon/expect-ct" Server: cloudflare CF-RAY: 4648a1b8bddc72e9-AMS

JSteunou commented 5 years ago

Are you using SockJS @Polve ?

Polve commented 5 years ago

No, I'm using:

Stomp.client(stompEndpoint, options)

JSteunou commented 5 years ago

In which browser?

Can you debug and tell me which value is ws.protocol here https://github.com/JSteunou/webstomp-client/blob/hotfix/protocol/src/client.js#L152

I still dont understand your bug, because when you do not set protocols webstomp-client uses ['v10.stomp', 'v11.stomp', 'v12.stomp'] as you can see in request header, then your server seems to send back 'v10.stomp' like if it chooses the 1st value just to please the client, but without matching the true stomp protocol version it uses. What make me say that is that when you force v1.2 from client side, it works, which make me think your server always uses v1.2 without taking http handshake in consideration.

There are differences between 1.0 and 1.2 stomp version that make them incompatible, which can lead to the issue you can see.

JSteunou commented 5 years ago

Can you also make a test when you set options.protocols to ['v11.stomp', 'v10.stomp', 'v12.stomp'] or ['v11.stomp', 'v12.stomp'] or even ['v99.stomp', 'v98.stomp'] to see if you server will send back the 1st item and your app still does not work. That will kind of confirm your server always send back the 1st protocol it sees, but always use v12.stomp underneath.

Polve commented 5 years ago

Can you debug and tell me which value is ws.protocol here https://github.com/JSteunou/webstomp-client/blob/hotfix/protocol/src/client.js#L152

It is 'v10.stomp' if I don't specify protocols, while I get 'v12.stomp' when I put that value in the protocol list

then your server seems to send back 'v10.stomp' like if it chooses the 1st value just to please the client, but without matching the true stomp protocol version it uses

It seems like it's a bit more complex (see below)

Can you also make a test when you set options.protocols to ['v11.stomp', 'v10.stomp', 'v12.stomp'] I receive v11.stomp

I tried with ['v99.stomp', 'v12.stomp'] and I got v12.stomp

While with ['v99.stomp', 'v98.stomp'] I get an empty string

JSteunou commented 5 years ago

Ok so it seems to at least check the validity of stomp version, but send the lowest or 1st protocol it finds.

You can use this check here https://github.com/JSteunou/webstomp-client/blob/master/src/client.js#L107 to verify if your server truly uses the version it tells using, or here for ack & nack https://github.com/JSteunou/webstomp-client/blob/master/src/client.js#L239

Marcsimm commented 5 years ago

It would really be appreciated if you guys could create a beta release of this fix. I see PR's that been up for years.

krusche commented 5 years ago

Thanks, it seems to work fine!