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

Integrate w/ HTTP.jl #84

Closed
quinnj opened this issue Dec 11, 2017 · 39 comments
Closed

Integrate w/ HTTP.jl #84

quinnj opened this issue Dec 11, 2017 · 39 comments

Comments

@quinnj
Copy link
Member

quinnj commented Dec 11, 2017

HTTP.jl is the consolidated, modernized, and currently actively maintained package for web functionality (uri parsing, content type detection, cookie handling, client requests, basic server functionality). It'd be great to have all the great work @hustf has done w/ this package lately work w/ that. Happy to discuss how to make that as easy as possible.

@hustf
Copy link
Collaborator

hustf commented Dec 11, 2017

It's nice to see some activity here! I am using Websockets for private projects, and think it's a great way for unlocking the door between Julia and Javascript powers.

I am thinking it would be a right move to switch for 0.7. As far as I recall now, I couldn't make a 0.7 version because of no response to a httpcommon pr (and some things that were not settled in 0.7 base.write). There are also some seemingly random delays in opening sockets from an unknown browser (Internet Explorer only, perhaps). Before you laugh too much, I am forced to using Internet Explorer for some work.

The old dispersed infrastructure makes sense with dedicated and competent owners, but that is lacking now. A good oportunity for a fresh start. I'd say switching here would require consensus including @MikeInnes, @shashi. Also, tests ought to include a) interfaces which are used in other packages already, b) the SSHL thingie, c) fully working Unix tests.

The second big issue here is including the ideas from Dandelion Websockets. With a good client, Websockets can be an interface between Julia processes, replacing Zmq and a small host of other technologies. But that increases the competence needed for maintaining this package. The current test set covers the common browser's client implementations.

@hgeorgako
Copy link

Looking forward to this work. Especially the Websockets (pure Julia) client integration into Websockets and/or HTTP.jl. For finance/trading related work, this is becoming an important technology to support.

@hustf
Copy link
Collaborator

hustf commented Dec 28, 2017

If we are to make this change, it would be really really helpful to have the tests running fully on the probably most common os first. I only use windows/ cygwin/ os x, and have not been able to figure out how to launch the browsers on linux. The tests pass with phantomjs headless browser, but that project is slowly dying.

@shashi
Copy link
Contributor

shashi commented Dec 29, 2017

I think this will be good to do since HTTP.jl is well maintained and future-proof!

@EricForgy
Copy link
Contributor

I need this functionality. If someone could outline some rough thoughts on how to proceed, maybe I can give it a try.

@hustf
Copy link
Collaborator

hustf commented Jan 7, 2018

That's great! Rough thoughts before looking much at HTTP.jl

  • Do this on 0.6 first.
  • Keep it on a branch
  • Import the functions from HTTP.jl that need to be modified.
  • When it works, we can modify HTTP and remove the import.

@EricForgy
Copy link
Contributor

HttpServer.Server explicitly has a websock field https://github.com/JuliaWeb/HttpServer.jl/blob/master/src/HttpServer.jl#L150.

HTTP.Server does not have such an explicit place to stick a websock https://github.com/JuliaWeb/HTTP.jl/blob/master/src/server.jl#L92

Would HTTP.Server need to be modified to include websock?

@EricForgy
Copy link
Contributor

EricForgy commented Jan 7, 2018

I see HTTP.Server has in::Channel and out::Channel, but I don't think a WebSocket fits this model, does it?

Edit: I take this back. It is probably fine 😅

@hustf
Copy link
Collaborator

hustf commented Jan 7, 2018

I'd have to learn more about channels first. Websockets just use sockets provided by HttpServer.

edit: I think you could say each open Websocket is a task initiated by, as it seems to WebSockets.jl, HttpServer. We would want to keep that kind of model at least, where Http takes over HttpServer's role.

@EricForgy
Copy link
Contributor

I'd have to learn more about channels first.

Same here 😅

@hustf
Copy link
Collaborator

hustf commented Jan 7, 2018

These things, tasks, channels, sockets, producers, are closely related terms at least. They say to work on one layer of software, you need a basic understanding of the deeper level, really. Hope I can learn something, too. And the IRC apps are a good way to get quick replies.

@hgeorgako
Copy link

There's this too: JuliaWeb/HTTP.jl#54

@hustf
Copy link
Collaborator

hustf commented Jan 8, 2018

Hm, I actually think it sounds more optimal to keep the websockets protocol in a separate package from Http. There are still going to be changes to the protocol in the future, and not least in the many different implementations of the client protocol out there. We still don't support the full protocol, and we still don't optimize fully for input / output of some buffers / types in Julia 0.7 (from my limited understanding).

For clarity, websockets can not be implemented except as an upgrade to other sockets. HttpServer does that now, and we are switching to Http.
@samoconnor, what's your take?

@samoconnor
Copy link

Hi @hustf, as @hgeorgako pointed out, I've implemented a WebSockets client (and test) in my HTTP.jl PR HTTP.jl branch: JuliaWeb/HTTP.jl#135. My motivation for doing this was that HTTP.jl did not have test coverage for the Upgrade: mechanism, and building a simple client for wss://echo.websocket.org seemed like the easiest way to understand how it is supposed to work and test how it interacts with connection pooling and pipelining.

As for division of functionality into packages I would prefer to see more separation rather than less. HTTP.jl currently aggregates code from a number of previous packages, which I think @quinnj would agree could eventually be split out again. Having now spent some time on the internals of HTTP.jl I can see why @quinnj found it necessary to combine the older packages before starting his mammoth refactoring and integration of his pure-julia parser.

My take at this point is that it is essential for HTTP.jl to include higher level protocols so that real-world tests can be implemented. For example my PR branch implements AWS authorisation to enable a test that does lots of concurrent GET/PUT against AWS S3. This enabled me to find intermittent bugs in HTTP.jl, MbedTLS.jl (JuliaLang/MbedTLS.jl#117, JuliaLang/MbedTLS.jl#114) and Base JuliaLang/julia#25314.

The refactoring I've done attempts to separate functionality into modules as much as possible within HTTP.jl to make it easier to eventually split it up into separate packages. e.g.

  • URI.jl
  • HTTPParser.jl
  • HTTPStream.jl
  • HTTPServer.jl
  • HTTPClient.jl
  • HTTPAuth.jl
  • HTTPCookies.jl

I think HTTP.jl should try to build up and maintain a test suite that includes stress testing against real-wold servers as well as unit tests. This probably means having a test/REQUIRE dependancy on the relevant client packages after everything has been split up.

@hustf
Copy link
Collaborator

hustf commented Jan 8, 2018

I'm not commenting on the general splitting / refactoring as I don't have the overview.

I'm impressed and especially glad to see you dig into the root causes of failures at high stress. The current tests for websockets deliberately reduce the simultaneous sockets. That was necessary most likely due to the issues you refer above. Travis don't like hard work anyway.

So testing as close as possible to the root cause makes a lot of sense. Benchmarks and stress tests are still important at the 'user level', especially with Websockets. Here, we want to optimize message size for responsiveness (or stock money in some cases?). I would certainly want to know if I can deploy on the web stably. But such tests should not be part of Pkg.test. They belong in /benchmark.

Few reviewers have your scope of knowledge and those can't do it all. The useful noobs most of all fear stirring up trouble for downstream packages, and like to maintain a few small parts. So how about we make a branch here (and elsewhere), and make multiple PRs toward those branches?

@samoconnor
Copy link

I've implemented a minimal HTTP.WebSockets.listen() server API here: HTTP.jl/src/WebSockets.jl as a test case for the HTTP.listen() API. I hope this helps to demonstrate a possible pattern for integrating this package with HTTP.jl.

Here is a simple example:

Server:

julia> HTTP.WebSockets.listen(;verbose=true) do ws
           while !eof(ws);
               print(String(readavailable(ws)))
           end
       end
I- Listening on: localhost:8081 -Info:HTTP.Nitrogen:server.jl:395
I- Accept:  🔗    0     0    localhost:8081:8081, 16, RawFD(22) -Info:HTTP.Nitrogen:server.jl:404
I- HTTP.Messages.Request:
|  """
|  GET / HTTP/1.1
|  Upgrade: websocket
|  Connection: Upgrade
|  Sec-WebSocket-Key: ywRsGHmJvybea7tq8G7qYA==
|  Sec-WebSocket-Version: 13
|  Host: localhost
|
|  """ -Info:HTTP.Nitrogen:server.jl:484
DEBUG: 9114 WebSocket ➡️  WebSocketHeader(TEXT | FINAL, 13-byte payload, mask = ede3bc6b)
Hello World!
DEBUG: 9114 WebSocket ➡️  WebSocketHeader(TEXT | FINAL, 19-byte payload, mask = c1ab71ee)
Hello World again!

Client:

julia> io = BufferStream()
julia> @async HTTP.WebSockets.open("ws://localhost:8081"; verbose=true) do ws
           write(ws, io)
       end

julia> write(io, "Hello World!\n")
DEBUG: 882313 WebSocket ⬅️  WebSocketHeader(TEXT | FINAL, 13-byte payload, mask = ede3bc6b)

julia> write(io, "Hello World again!\n")
DEBUG: 882319 WebSocket ⬅️  WebSocketHeader(TEXT | FINAL, 19-byte payload, mask = c1ab71ee)

Here is an example with 2 way comms where: the client process streams audio data from the web and the server process streams it to vlc; and the server sends status messages back to the client:

┌───────────────────────────────────────────┐
│                                           │
│ http://tinyurl.com/bach-cello-suite-1-ogg │
│                                           │
└─────────────────────┬─────────────────────┘
                      │
           audio data │
                      │
┌─────────────────────▼─────────────────────┐
│                                           │
│       HTTP.open("GET", ...) do ...        │
│                                           │
└─────────────────────┬─────────────────────┘
                      │
┌─────────────────────▼─────────────────────┐         ┌────────────────┐
│                                           │         │                │
│     HTTP.WebSockets.open(...) do ...  ┌───┼────────▶│   println()    │
│                                       │   │         │                │
└─────────────────────┬─────────────────┼───┘         └────────────────┘
                      │                 │
                      │                 │ status messages
                      │                 │
┌─────────────────────▼─────────────────┴───┐
│                                           │
│      HTTP.WebSockets.listen() do ...      │
│                                           │
└─────────────────────┬─────────────────────┘
                      │
┌─────────────────────▼─────────────────────┐
│                                           │
│   open(`vlc --play-and-exit -`) do ...    │
│                                           │
└───────────────────────────────────────────┘

P.S. monodraw is fun!

Server:

using HTTP; HTTP.WebSockets.listen() do ws
           open(`vlc -q --play-and-exit --intf dummy -`, "w") do vlc_stdin
               while !eof(ws)
                   ogg_samples = readavailable(ws)
                   write(vlc_stdin, ogg_samples)
                   write(ws, "Streamed $(length(ogg_samples))-bytes...")
               end
           end
       end
I- Listening on: localhost:8081 -Info:HTTP.Nitrogen:server.jl:395
I- Accept:  🔗    0     0    localhost:8081:8081, 16 -Info:HTTP.Nitrogen:server.jl:404
VLC media player 2.0.9 Twoflower (revision 2.0.9-1-g8ae4dde)
DEBUG: 60d2 WebSocket ➡️  WebSocketHeader(TEXT | FINAL, , mask = f59bcad2)
DEBUG: 60d2 WebSocket ⬅️  WebSocketHeader(TEXT | FINAL, 19-byte payload, mask = 3be2b3d6)
DEBUG: 60d2 WebSocket ➡️  WebSocketHeader(TEXT | FINAL, 328-byte payload, mask = e5664e58)
DEBUG: 60d2 WebSocket ⬅️  WebSocketHeader(TEXT | FINAL, 21-byte payload, mask = 1881c8ad)
DEBUG: 60d2 WebSocket ➡️  WebSocketHeader(TEXT | FINAL, 1300-byte payload, mask = 444ecb1)
DEBUG: 60d2 WebSocket ⬅️  WebSocketHeader(TEXT | FINAL, 22-byte payload, mask = acb4d4d5)
DEBUG: 60d2 WebSocket ➡️  WebSocketHeader(TEXT | FINAL, 1300-byte payload, mask = 8e1a56a5)

Client:

julia> using HTTP; HTTP.WebSockets.open("ws://localhost:8081") do ws
           HTTP.open("GET", "http://tinyurl.com/bach-cello-suite-1-ogg"; verbose=2) do http
               while !eof(http)
                   write(ws, readavailable(http))
                   println(String(readavailable(ws)))
               end
           end
       end
DEBUG: d7f0 🔗  New:            🔗    0     0    localhost:8081:53264, 16
HTTP.Messages.Request:
"""
GET /bach-cello-suite-1-ogg HTTP/1.1
Host: tinyurl.com
Content-Length: 0

"""
DEBUG: d7f0 WebSocket ⬅️  WebSocketHeader(TEXT | FINAL, , mask = f59bcad2)
DEBUG: d7f0 WebSocket ➡️  WebSocketHeader(TEXT | FINAL, 19-byte payload, mask = 3be2b3d6)
Streamed 0-bytes...
HTTP.Messages.Response:
"""
HTTP/1.1 301 Moved Permanently
Date: Thu, 11 Jan 2018 21:50:57 GMT
Content-Type: text/html; charset=UTF-8
Location: https://upload.wikimedia.org/wikipedia/commons/4/43/JOHN_MICHEL_CELLO-J_S_BACH_CELLO_SUITE_1_in_G_Prelude.ogg

"""
DEBUG: d7f0 ➡️  Redirect: https://upload.wikimedia.org/wikipedia/commons/4/43/JOHN_MICHEL_CELLO-J_S_BACH_CELLO_SUITE_1_in_G_Prelude.ogg
DEBUG: d7f0 🔗  New:0     0    upload.wikimedia.org:443:53265, 16
HTTP.Messages.Request:
"""
GET /wikipedia/commons/4/43/JOHN_MICHEL_CELLO-J_S_BACH_CELLO_SUITE_1_in_G_Prelude.ogg HTTP/1.1
Host: upload.wikimedia.org
Content-Length: 0

"""
DEBUG: d7f0 WebSocket ⬅️  WebSocketHeader(TEXT | FINAL, 328-byte payload, mask = e5664e58)
DEBUG: d7f0 WebSocket ➡️  WebSocketHeader(TEXT | FINAL, 21-byte payload, mask = 1881c8ad)
Streamed 328-bytes...
DEBUG: d7f0 WebSocket ⬅️  WebSocketHeader(TEXT | FINAL, 1300-byte payload, mask = 444ecb1)
DEBUG: d7f0 WebSocket ➡️  WebSocketHeader(TEXT | FINAL, 22-byte payload, mask = acb4d4d5)
Streamed 1300-bytes...
DEBUG: d7f0 WebSocket ⬅️  WebSocketHeader(TEXT | FINAL, 1300-byte payload, mask = 8e1a56a5)
DEBUG: d7f0 WebSocket ➡️  WebSocketHeader(TEXT | FINAL, 22-byte payload, mask = 3f494a3d)
Streamed 1300-bytes...
DEBUG: d7f0 WebSocket ⬅️  WebSocketHeader(TEXT | FINAL, 1300-byte payload, mask = e0631c92)

@EricForgy
Copy link
Contributor

This looks super encouraging 🙌

If I understand, this means WebSockets can be implemented here without any change to HTTP.jl. Is that right?

WebSockets.jl here is 399 lines of code. WebSockets.jl in HTTP.jl is 297 lines of code.

Naive question: What is standing in the way of just adopting the HTTP.WebSockets stuff here?

@samoconnor
Copy link

@EricForgy I assume that this package has a more complete implementation of WebSockets than my quick attempt. I haven't looked into the code in this package enough to know exactly what extra features it has. You are welcome to try out HTTP.WebSockets directly (use the examples above as a starting point).
I'll leave it to the maintainers of this package to decide how to integrate things. Please copy/paste whatever parts of HTTP.WebSockets you think are useful.

@EricForgy
Copy link
Contributor

EricForgy commented Jan 20, 2018

Anyone up for the challenge?? I'm not particularly skilled at this stuff, but as I mentioned above, I need it, so gonna take a stab at it 😅

I've now spent some time with HTTP.jl and excited to see @samoconnor 's massive PR merged 🎉

As I understand it, WebSockets.jl was originally based on HttpServer.jl. There are three elements from HttpServer that need to be implemented in WebSockets.jl

  1. WebSocketHandler <: HttpServer.WebSocketInterface
  2. handle
  3. is_websocket_handshake

I doubt you'll want to remove HttpServer from WebSockets, so we'll need to support both for a while, which means REQUIRE will need HTTP included.

My initial thought is to just mimic the current situation, i.e. have three elements from HTTP.jl that need to be implemented in WebSockets.jl:

  1. WSHandler <: HTTP.WebSockets,WebSocketInterface
  2. handle
  3. is_websocket_handshake

On the HTTP.jl side, we can probably just add a new listen method that accepts both HTTP and WS handlers.

How does that sound for a start?

PS: I'll be working on this on v0.6.2.

Update: Adding using HTTP doesn't break anything 😃

@EricForgy
Copy link
Contributor

OK... had a quick look and am considering a new game plan.

HTTP.WebSockets actually looks pretty complete, so I am thinking about leaving WebSockets.jl to HttpServer.jl and just focus on improving HTTP.WebSockets.jl.

Then when the eventual "re-split up" of HTTP.jl into modules happens that @samoconnor mentioned above ( #84 (comment) ), we can also split HTTP.WebSockets.jl back into this package and eventually deprecate HttpServer.

What do you think @samoconnor ?

@hustf
Copy link
Collaborator

hustf commented Jan 20, 2018

Eric, thanks for the effort. Just some side info.

A HttpCommon PR mentioned above got merged to master this morning. I had websockets running locally on 0.7 based off this PR. The PR puts some now probably unnecessary restrictions on types (no AbstractString) that I believe leads to more memory operations than necessary in some cases. TL;DR HtttpCommon is not dead yet. This is an argument for going the way you suggest in the previous post.

Actually, if there in the end is an optimized package supporting WebSockets , we could add more userfriendliness to this package. When using WebSockets, I normally include a verbose logging module that helps tracking down errors and aids understanding. The logging can be turned off, but there will be a slight speed penalty. Websockets can be made more user friendly if there is another package that takes responsibility for the cutting edge speed. That's another argument for what you propose.

The counter-arguments are not very strong: 1) It would be a nice challenge to make the switch in a seamless way. It doesn't sound that hard to me. And, 2) I may be right in assuming that the socket-opening-problems we see share its root-cause with Http.

@EricForgy
Copy link
Contributor

Hi guys 👋

I am making some progress ( 🎉 ) but having a little trouble with one of the last steps ( 😅 ).

This is an issue with HTTP.WebSockets and not this WebSockets, but I am putting a plea to all experts here for help 🙏

Any ideas? JuliaWeb/HTTP.jl#176

@EricForgy
Copy link
Contributor

EricForgy commented Feb 12, 2018

Hi guys 👋

By now, I spent enough time looking at HTTP.WebSockets (while pulling my hair out 👴 ), I could probably write my own WS package in my sleep by now 😅

I have a fresh perspective now. I think it is amazing @samoconnor wrote HTTP.WebSockets almost as a byproduct of needing to test upgrade. That is a testament.

However, I see some advantages of this package. I am coming full circle. I may integrate this package with HTTP.jl next.

One side effect would be this package would have a dependency on both HTTP.jl and HttpServers.jl.

That doesn't seem to be great.

Idea....

What if we move the few lines of code in WebSockets.jl that depend on HttpServer.jl INTO HttpServer.jl so that WebSockets.jl doesn't depend on either HttpServer OR HTTP. We could then add just the pieces that HTTP needs into HTTP.

@EricForgy
Copy link
Contributor

EricForgy commented Feb 13, 2018

This still needs some work, but I think it is a good start 😊

#87

It has one dependency: HTTP.jl.

And you can use WebSockets as both a server and client. I fixed up some of the TODOs listed in the comments as well. It is a breaking change though (obviously) :)

There are just a few simple tests, but they are passing 👍

using WebSockets
using WebSockets.HTTP
using HTTP.IOExtras
using Base.Test

import WebSockets: CONNECTED, CLOSING, CLOSED

@testset "WebSockets" begin

for s in ["ws"] # ["ws", "wss"]
    info("Testing $(s)...")
    WebSockets.open("$s://echo.websocket.org") do ws
        write(ws, "Foo")
        @test String(read(ws)) == "Foo"

        close(ws)
    end
end


p = UInt16(8000)
@async HTTP.listen("127.0.0.1",p) do http
    if WebSockets.is_upgrade(http.message)
        WebSockets.upgrade(http) do ws
            while ws.state == CONNECTED
                data = String(read(ws))
                write(ws,data)
            end
        end
    end
end

sleep(2)

info("Testing local server...")
WebSockets.open("ws://127.0.0.1:$(p)") do ws
    write(ws, "Foo")
    @test String(read(ws)) == "Foo"

    write(ws, "Bar")
    @test String(read(ws)) == "Bar"

    close(ws)
end

end # testset

What do you think?

Update: I also submitted a PR to HTTP.jl to use this revised WebSockets.jl.

JuliaWeb/HTTP.jl#197

@hustf
Copy link
Collaborator

hustf commented Feb 13, 2018

Merged #87 to a feature branch 'change_dependencies'. More commits / PRs are needed before this replaces the current WebSockets.

Looking forward to a closer look in a few days! Hard to have opinions before that.

@EricForgy
Copy link
Contributor

EricForgy commented Feb 13, 2018

Great. I would think before we can even consider a merge, we probably want the most basic sample to work as presented in the README (minus using HttpServer), i.e.

using WebSockets

wsh = WebSocketHandler() do req,client
    while true
        msg = read(client)
        write(client, msg)
    end
  end

server = Server(wsh)
run(server,8080)

Well, maybe not "as presented". We might want to remove the req argument since it isn't needed. Instead of Server -> run, we might just make it listen(wsh,8080). Some breaking changes would be ok, right?

It would probably be nice if the examples worked too.

I haven't looked through all the tests, but it seems to be a lot. Not sure how much work is required before we can replace any broken tests.

Anything else we can say even before reviewing the code?

Edit: Also, I think very little of WebSockets actually depends on HTTP or HttpServer. What if we just strip all dependencies out or maybe have WebSockets check to see if HTTP or HttpServer is loaded and supply the respective few supported functions accordingly.

@EricForgy
Copy link
Contributor

EricForgy commented Feb 14, 2018

I've started working my way through the tests. So far so good 😊

image

I basically removed WebSockets dependency on BOTH HttpServer and HTTP and load methods depending on whether you are using HTTP or HttpServer

julia> using HTTP

julia> using WebSockets
INFO: Loading HTTP methods...

or conversely

julia> using HttpServer

julia> using WebSockets
INFO: Loading HttpServer methods...

Update: Actually, when I uncomment the rest of the tests, all tests pass. There are some error messages printed to screen, but it didn't seem to affect the actual tests. Will keep looking...

Update^2: New PR 😊 #88

@hustf
Copy link
Collaborator

hustf commented Feb 14, 2018

The rest of the tests are very verbose and could definitely be improved.

About breaking changes. This is acceptable when the (undocumented) functionality like req arguments are available through Http(server).

@EricForgy
Copy link
Contributor

EricForgy commented Feb 14, 2018

I'm curious if there are ANY breaking changes now 😊

I removed the id property for the WebSocket object because @samoconnor convinced me we don't need it and that should be handled by the user if they want to keep track. That would be breaking change if anyone is relying on id, which is likely I guess, but easy to fix on the user's end. I can update the example to show how it can work with id in the object.

Edit: For example, this

using HttpServer
using WebSockets

wsh = WebSocketHandler() do req,client
    while true
        msg = read(client)
        write(client, msg)
    end
  end

server = Server(wsh)
run(server,8080)

works as is now.

Update: Oops. Still needs some work 😅 I'll try to get the chat example working...

Update^2: Found the problem 😊 Will fix and add a new commit to #88

@hustf
Copy link
Collaborator

hustf commented Feb 14, 2018 via email

@EricForgy
Copy link
Contributor

Sure. Most applications need some way to keep track of threads / sessions, but it doesn't need to be in the WebSocket object per se as not all applications need that. I'll fix the chat example and then have a look at the browser tests.

@EricForgy
Copy link
Contributor

EricForgy commented Feb 14, 2018

The chat example is working now 👍 😊

It still keeps track of the clients, but we don't need an id in the WebSocket object. I think this is cleaner and leaves the implementation to the user.

Update: I'll take a look at the browser tests later, but it is evening in Manila and my wife will kill me if I don't get off my computer 😅 💟

This was referenced Feb 14, 2018
@EricForgy
Copy link
Contributor

Both chat.jl and server.jl examples working now in latest commit in #88 . Working on browser tests.

@EricForgy
Copy link
Contributor

EricForgy commented Feb 15, 2018

Hi guys 👋 @samoconnor , @quinnj et al, I have a question.

I was working through the browser tests and one change I need to make everywhere is to remove ws.id since the new WebSocket type does not have an id property. To identify the client, I have been sending an id generated on the client side in a JSON string. This works fine for sending text frames, but now I am working on a test that sends a binary frame. I still need to identify the client somehow, but can't do it the same way with a JSON string since JSON doesn't support binary (right?).

Any ideas?

One thought is that I can send the id from the client in one frame but set the fin bit to zero so the server knows there is another frame coming and make that frame binary.

How would you do it?

Edit: Still curious to learn how you would do it, but I hacked something together that seems to work. Everything goes through except the very last browser test:

Test Failed
  Expression: n_msgs == n_responders * 37
   Evaluated: 35 == 37

It is probably fine. Just missed a count somewhere 👍

@hustf
Copy link
Collaborator

hustf commented Feb 15, 2018

Congrats on the progress! Hope to check this out in the weekend.

There are obvious ways of assigning sequential ids to each connection, but that's not what you're asking I think. It is very useful to identify two or more websockets as being opened from the same browser and the same document instance. Even the failing tests are difficult to correlate with browser without that info. That's part of the reason tests were made sequential in a revision.

The 'old' binary browser tests transmitted the current image size using numbers in an ad hoc way using just 'reinterpret()'. There was also a comment 'don't do this at home' because I did not have the time to figure out how hton() and bswap() work, and the whole history of endianness with different OSs. Stuff like this is why we do need CI tests. As long as it works, some of us are not really interested in bits and bytes order.

So the quick and dirty way to provide identifiers for the binary tests would be to extend the headers currently used for image info.

Since the websockets are initiated client side, I think the cleaner way to provide an id would be have javascript generate a random semi-unique ID number (https://stackoverflow.com/questions/105034/create-guid-uuid-in-javascript), and open a text socket from the same browser page. The text channel can transmit the same id number as the binary channel(s), along with the browser id (copy the browser id function from the first page). Output could be sorted by the random id at the end.

The same approach would also benefit the text-only tests, which open three websockets from each page.

Now, do we also need a way to combine individual http requests with individual websockets? I don't think so. Each browser has its own solution it seems, and opens lots of sockets per page in some cases. For the 'old' tests I used an older http protocol to keep the number of sockets down to something httpserver could manage without so many errors. Nobody else will do that.

We could previously combine http request with websocket call in several ways: By inspecting the request field in the handler call, by looking at the socket id's, by looking at the socket id's in the 'connect' event (before upgrade) or by writing to a cookie. We retain the last solution by having javascript include a unique id in the http request or in a cookie as well as through the websockets. All these solutions are a bit complicated I think, and not relevant to testing Websockets as such.

@EricForgy
Copy link
Contributor

Hi @hustf 👋

Since the websockets are initiated client side, I think the cleaner way to provide an id would be have javascript generate a random semi-unique ID number (https://stackoverflow.com/questions/105034/create-guid-uuid-in-javascript), and open a text socket from the same browser page.

This is precisely what I am doing.

function uuid4() {
    return ([1e7]+-1e3+-4e3+-8e3+-1e11).replace(/[018]/g, c =>
        (c ^ crypto.getRandomValues(new Uint8Array(1))[0] & 15 >> c / 4).toString(16)
    )
}
var id = uuid4();

I think this is the way to go 😊

The 'old' binary browser tests transmitted the current image size using numbers in an ad hoc way using just 'reinterpret()'.

I see this now. What I did that felt hackish is maybe not so bad. I just have my handler for the specified protocol read twice. It first sends the id as a text from the browser and then sends the binary payload. There might be a better way, but this is probably better than sending a 6-byte array with an if statement in the handler with a friendly warning 😊

I'm feeling pretty good about the state so far. I could add some more sugar so the user doesn't need to worry about the upgrade stuff when using HTTP.jl. It could be the same syntax as HttpServer.jl.

@EricForgy
Copy link
Contributor

I had a look at Pkg.dependents("WebSockets") and found four METADATA packages that depend on WebSockets.jl (understand there are likely more private repos, but hard to do or say anything about that).

I briefly looked at all four and don't see anything that the new WebSocket object would break. Here are the results of running the respective tests.

Mux.jl

Tests pass with no problem.

Escher.jl

Tests do not pass but seem unrelated to WebSockets.jl.

ERROR: LoadError: LoadError: LoadError: MethodError: Cannot `convert` an object of type Symbol to an object of type AbstractString
This may have arisen from a call to the constructor AbstractString(...),
since type constructors fall back to convert methods.
Stacktrace:
 [1] Type at .\<missing>:0 [inlined]
 [2] #=>#51 at C:\Users\Eric\.julia\v0.6\Escher\src\basics\macros.jl:223 [inlined]
 [3] =>(::Symbol, ::String) at C:\Users\Eric\.julia\v0.6\Escher\src\basics\macros.jl:223
 [4] include_from_node1(::String) at .\loading.jl:576
 [5] include(::String) at .\sysimg.jl:14
 [6] include_from_node1(::String) at .\loading.jl:576
while loading C:\Users\Eric\.julia\v0.6\Escher\src\basics/util.jl, in expression starting on line 59
while loading C:\Users\Eric\.julia\v0.6\Escher\src\Escher.jl, in expression starting on line 27
while loading C:\Users\Eric\.julia\v0.6\Escher\test\runtests.jl, in expression starting on line 1

I get the same error when using the latest tagged version of WebSockets.jl.

HeadlessChromium.jl

Tests do not pass but seem unrelated to WebSockets.jl.

The failure is related to WebSockets, but doesn't look related to WebSockets.jl. There is some internal implementation. When I run the tests with the latest tagged version of WebSockets.jl, the tests don't pass either.

Blink.jl

Tests pass with no problem.

@EricForgy
Copy link
Contributor

More progress 😊

I cleaned up the close method The tests were passing but there were some residual messages spewing to the REPL after the test completed. That is cleaned up now 👍

Something I think is pretty cool is that now we can run BOTH an HTTP server and an HttpServer server simulataneous and interact with both using the new WebSocket client capability. This opens up all kinids is possibilities for simpler testing.

using HTTP
using HttpServer
using WebSockets
using Base.Test

@testset "HTTP" begin

info("Testing ws...")
WebSockets.open("ws://echo.websocket.org") do ws
    write(ws, "Foo")
    @test String(read(ws)) == "Foo"

    close(ws)
end
sleep(1)

info("Testing wss...")
WebSockets.open("wss://echo.websocket.org") do ws
    write(ws, "Foo")
    @test String(read(ws)) == "Foo"

    close(ws)
end
sleep(1)

port_HTTP = 8000
info("Start HTTP server on port $(port_HTTP)")
@async HTTP.listen("127.0.0.1",UInt16(port_HTTP)) do http
    if WebSockets.is_upgrade(http.message)
        WebSockets.upgrade(http) do ws
            while !eof(ws)
                data = String(read(ws))
                write(ws,data)
            end
        end
    end
end

port_HttpServer = 8080
info("Start HttpServer on port $(port_HttpServer)")
wsh = WebSocketHandler() do req,ws
    while !eof(ws)
        msg = String(read(ws))
        write(ws, msg)
    end
end
server = Server(wsh)
@async run(server,port_HttpServer)

sleep(2)

info("Testing local HTTP server...")
WebSockets.open("ws://127.0.0.1:$(port_HTTP)") do ws
    write(ws, "Foo")
    @test String(read(ws)) == "Foo"

    write(ws, "Bar")
    @test String(read(ws)) == "Bar"

    close(ws)
end

info("Testing local HttpServer...")
WebSockets.open("ws://127.0.0.1:$(port_HttpServer)") do ws
    write(ws, "Foo")
    @test String(read(ws)) == "Foo"

    write(ws, "Bar")
    @test String(read(ws)) == "Bar"

    close(ws)
end

end # testset

All tests above are passing 😊

@hustf
Copy link
Collaborator

hustf commented Jun 6, 2018

This has taken a long time to finally merge, but since we're still supporting some good tools with this package it would be a pity to introduce any obscure bugs. Testing coverage, is now at 96% and I believe this is an improvement to quality as well as functionality. A big thank you to you Eric Forgy, Jacob Quinn and Sam o' Connor!

With extensive testing in place, the threshold to making new changes to core functionality will be much lower. We can very soon start moving to 0.7, and considering where to go with logging. I believe networked applications have some specific requirements to logging that are basically the same as for example ZMQ.

Exception conditions and error handling testing is much more thorough than before. This has a speed cost. We added benchmarks first, in order to measure that cost, which is hardly noticeable. We have also (perhaps temporarily) disabled browser tests.

In the meantime, @quinnj and others has continued to improve Http.jl. There is also work going on in DandelionWebSockets.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

6 participants