Giter VIP home page Giter VIP logo

Comments (12)

timcoote avatar timcoote commented on August 16, 2024

I've checked the latest update and worked through the websocket-client changes between 0.4.1 and the current version (0.32.0), both in the tests and with the dev team for websocket-client. There are two changes that break the tests: the name of an exception has changed and the value returned from websockets when a connection disappears has changed from None to "". Ironically, the feedback from the websocket-client team was that None is probably a more sensible return value, but the newer version is now too widely deployed to change the API contract, again.

From my pov, the sockjs stuff now all works over ipv6.

fwiw, here's my patch for the change to this repo - although I favour pinning the package versions, rather than simply pulling the latest as my patch implies:

diff --git a/requirements.txt b/requirements.txt
index f838ad3..8c734b3 100644
--- a/requirements.txt
+++ b/requirements.txt
@@ -1,3 +1,3 @@
 unittest2
-websocket-client==0.4.1
-ws4py
\ No newline at end of file
+websocket-client #==0.4.1
+ws4py
diff --git a/sockjs-protocol.py b/sockjs-protocol.py
index e6e8a3d..a5afc4f 100644
--- a/sockjs-protocol.py
+++ b/sockjs-protocol.py
@@ -543,9 +543,9 @@ class WebsocketHixie76(Test):
         self.assertEqual(ws.recv(), u'c[3000,"Go away!"]')

         # The connection should be closed after the close frame.
-        with self.assertRaises(websocket.ConnectionClosedException):
-            if ws.recv() is None:
-                raise websocket.ConnectionClosedException
+        with self.assertRaises(websocket.WebSocketConnectionClosedException):
+            if ws.recv() is "":
+                raise websocket.WebSocketConnectionClosedException
         ws.close()

     # Empty frames must be ignored by the server side.
@@ -639,9 +639,9 @@ class WebsocketHixie76(Test):
         ws = websocket.create_connection(ws_url)
         self.assertEqual(ws.recv(), u'o')
         ws.send(u'["a')
-        with self.assertRaises(websocket.ConnectionClosedException):
-            if ws.recv() is None:
-                raise websocket.ConnectionClosedException
+        with self.assertRaises(websocket.WebSocketConnectionClosedException):
+            if ws.recv() is "":
+                raise websocket.WebSocketConnectionClosedException
         ws.close()

from sockjs-protocol.

brycekahle avatar brycekahle commented on August 16, 2024

As mentioned in #86, we don't want to upgrade websocket-client because it used to test Hixie/Draft 76 compatibility. If you can find another module to use to test ipv6, I would welcome that change.

from sockjs-protocol.

timcoote avatar timcoote commented on August 16, 2024

I believe that the patch makes the Hixie-76 tests pass - I had two errors in test_closed () and test_broken_json (), which were both due to the exception name change from ConnectionClosedException to WebSocketConnectionClosedException.

When the name change is fixed, both tests fail due to the change in semantics of the returned value (ie utils.py's WebSocket8Client.recv() returns an empty string, rather than None.), which is the change in tested returned value.

So I don't think that there's anything else to do. If there is, pls explain and I'll see if I can have a crack at it.

On the sockjs-node end, I'm using the 'make test_server' and changing the line in config.js:

git diff 4561f500e92
diff --git a/tests/test_server/config.js b/tests/test_server/config.js
index 415e1eb..d5ae686 100644
--- a/tests/test_server/config.js
+++ b/tests/test_server/config.js
@@ -5,5 +5,5 @@ exports.config = {
     },

     port: 8081,
-    host: '0.0.0.0'
+    host: '::'
 };

The test_server then listens on both ipv4 and ipv6, although I've not built automated tests to confirm that, which I would do to support a patch.

fwiw, the tests run end to end (with a fixed dns server) with the server accessed directly, or via an intermediate ipv4 host, which routes to ipv6 with a simple socat application bridge.

Newer versions of the various dependent libraries now seem to support stuff like parsing ipv6 addresses, handling different dns connections, etc, so ipv6 'just works'. Test hosts are fedora 21 for both ends.

from sockjs-protocol.

brycekahle avatar brycekahle commented on August 16, 2024

Upgrading websocket-client means it no longer uses Hixie-76, which is why we can't upgrade it.

from sockjs-protocol.

timcoote avatar timcoote commented on August 16, 2024

ok. I clearly don't understand what I'm looking at - not much change there, then :-)

 venv/bin/python sockjs-protocol.py WebsocketHixie76
......
----------------------------------------------------------------------
Ran 6 tests in 0.092s

OK

and on sockjs-node:

 make test_server
./node_modules/.bin/coffee -o lib/ -c src/*.coffee
node tests/test_server/server.js
SockJS v0.3.15 bound to "/echo"
SockJS v0.3.15 bound to "/disabled_websocket_echo"
SockJS v0.3.15 bound to "/cookie_needed_echo"
SockJS v0.3.15 bound to "/close"
SockJS v0.3.15 bound to "/ticker"
SockJS v0.3.15 bound to "/amplify"
SockJS v0.3.15 bound to "/broadcast"
 [*] Listening on :::8081
GET /echo/000/07285fff-78c9-4fc3-b0bf-b86b28ba2d83/websocket 7ms (unfinished)
    [+] echo open    <SockJSConnection d621f698-1dcc-40d9-bbda-c689e2abf2e0>
    [-] echo close   <SockJSConnection d621f698-1dcc-40d9-bbda-c689e2abf2e0>
GET /close/000/858f6238-8bde-4d31-ba90-fb48bef4057b/websocket 3ms (unfinished)
    [+] clos open    <SockJSConnection 1eb7928a-5e00-42ac-a6a5-e59f4bd16781>
GET /echo/000/98a437fb-e9d1-4858-8a74-855ace108c24/websocket 1ms (unfinished)
    [+] echo open    <SockJSConnection ff9ceb21-a74a-432b-9a9e-e51cbb391e5e>
    [ ] echo message <SockJSConnection ff9ceb21-a74a-432b-9a9e-e51cbb391e5e> "a"
    [-] echo close   <SockJSConnection ff9ceb21-a74a-432b-9a9e-e51cbb391e5e>
GET /echo/000/66bdc4aa-0a02-40ad-b3d2-bd9ba6f52795/websocket 1ms (unfinished)
    [+] echo open    <SockJSConnection 1a2f9ac9-6f1a-4608-b386-c44309529660>
    [-] echo close   <SockJSConnection 1a2f9ac9-6f1a-4608-b386-c44309529660>
GET /echo/000/c23837ee-ed39-4734-9fc0-f20964bc74e0/websocket 1ms (unfinished)
    [+] echo open    <SockJSConnection 30f9e70b-1087-4d9c-a172-6e1eb9abf9ed>
GET /echo/000/c23837ee-ed39-4734-9fc0-f20964bc74e0/websocket 0ms (unfinished)
    [+] echo open    <SockJSConnection 77c7c517-79d8-4e34-8b26-87d585e083e3>
    [ ] echo message <SockJSConnection 30f9e70b-1087-4d9c-a172-6e1eb9abf9ed> "a"
    [ ] echo message <SockJSConnection 77c7c517-79d8-4e34-8b26-87d585e083e3> "b"
    [-] echo close   <SockJSConnection 30f9e70b-1087-4d9c-a172-6e1eb9abf9ed>
    [-] echo close   <SockJSConnection 77c7c517-79d8-4e34-8b26-87d585e083e3>
GET /echo/000/c23837ee-ed39-4734-9fc0-f20964bc74e0/websocket 0ms (unfinished)
    [+] echo open    <SockJSConnection 5128a6cf-e057-4219-b845-40e73e5201f4>
    [ ] echo message <SockJSConnection 5128a6cf-e057-4219-b845-40e73e5201f4> "a"
    [-] echo close   <SockJSConnection 5128a6cf-e057-4219-b845-40e73e5201f4>
GET /echo/000/4ec205c8-5c76-4adb-9770-1e37fa0c3aaa/websocket 3ms (unfinished)
    [+] echo open    <SockJSConnection f7cacbd0-99f9-4a29-bfe2-e09a55dc4501>
    [ ] echo message <SockJSConnection f7cacbd0-99f9-4a29-bfe2-e09a55dc4501> "a"
    [-] echo close   <SockJSConnection f7cacbd0-99f9-4a29-bfe2-e09a55dc4501>

what would I need to do for the ipv6 validation patch?

from sockjs-protocol.

brycekahle avatar brycekahle commented on August 16, 2024

If you look at https://github.com/liris/websocket-client it says

websocket-client supports only hybi-13

but, if you go back to the version we are using in the tests, 0.4.1, it does not say that, because it is using Hixie-76. So the reason we are using the old version is test our compatibility with Hixie-76.

I think if we wanted to update the tests to support IPv6, we would need to find a module that supports both Hixie-76 and IPv6.

from sockjs-protocol.

timcoote avatar timcoote commented on August 16, 2024

ok. I see what you mean now. Presumably the Hixie tests are being swallowed somewhere in the webclient code and should be failing.

Do you collect stats on which protocol versions are actually used - although it makes sense to support widely used versions, it's a killer to support all old versions and drains a lot of effort from supporting new stuff. An approach could be to not support Hixie-76 on ipv6.

That said, I seem to be the only person wanting ipv6, so my views should be discounted.

I'll see if I can spot a websocket client library that supports both ipv6 and Hixie-76.

from sockjs-protocol.

brycekahle avatar brycekahle commented on August 16, 2024

A main use case of sockjs is to support older browsers and transports, so keeping support for Hixie-76 is desired. This combo of Hixie-76 and IPv6 support will also extend to any of the server implementations beyond these tests, so having the tests verify correct behavior is important.

I do think IPv6 support is desired, but not at the cost of other features. I will try and do some research to find a way forward.

from sockjs-protocol.

timcoote avatar timcoote commented on August 16, 2024

ok. I must say, now that I've read a bit more of the background, different protocol version support looks like a product management nightmare.

Happy to help if I can.

from sockjs-protocol.

timcoote avatar timcoote commented on August 16, 2024

support's not moved to faye-websocket has it? I had some issues with getting the compatible versions of websocket-client and faye-websocket sorted out (cruft left by the build process), and faye-websocket has references to Hixie-76 in it. I'm not clear how these components interact.

from sockjs-protocol.

timcoote avatar timcoote commented on August 16, 2024

doh! that's on the node end!

from sockjs-protocol.

timcoote avatar timcoote commented on August 16, 2024

This page has some stats on protocol support for browsers: http://bit.ly/1JgKR3z. If it's correct then ~86% of browsers in use support the newer versions and <1% the older ones. I don't know the source data, but I have used netcraft for distributions in the past. Of course, this doesn't identify the actual market use and it could well be that there are important subgroups that require older protocol support.

from sockjs-protocol.

Related Issues (20)

Recommend Projects

  • React photo React

    A declarative, efficient, and flexible JavaScript library for building user interfaces.

  • Vue.js photo Vue.js

    🖖 Vue.js is a progressive, incrementally-adoptable JavaScript framework for building UI on the web.

  • Typescript photo Typescript

    TypeScript is a superset of JavaScript that compiles to clean JavaScript output.

  • TensorFlow photo TensorFlow

    An Open Source Machine Learning Framework for Everyone

  • Django photo Django

    The Web framework for perfectionists with deadlines.

  • D3 photo D3

    Bring data to life with SVG, Canvas and HTML. 📊📈🎉

Recommend Topics

  • javascript

    JavaScript (JS) is a lightweight interpreted programming language with first-class functions.

  • web

    Some thing interesting about web. New door for the world.

  • server

    A server is a program made to process requests and deliver data to clients.

  • Machine learning

    Machine learning is a way of modeling and interpreting data that allows a piece of software to respond intelligently.

  • Game

    Some thing interesting about game, make everyone happy.

Recommend Org

  • Facebook photo Facebook

    We are working to build community through open source technology. NB: members must have two-factor auth.

  • Microsoft photo Microsoft

    Open source projects and samples from Microsoft.

  • Google photo Google

    Google ❤️ Open Source for everyone.

  • D3 photo D3

    Data-Driven Documents codes.