HTTP.jl v0.8.0

[EricForgy]

Let's have an open issue to discuss this effort until we get a version merged and tagged that supports HTTP.jl v0.8.0.

It looks like several days after I started work on this, a new branch was merged and tagged and I didn't notice 😅 Let's communicate what we're working on here to avoid duplicate efforts.

Since I already had a branch with all tests passing for HTTP.jl v0.8.0, I went ahead and started work on PlotlyJS.jl and WebIO.jl and have local working copies of those. Now I need to figure out how to get the new HTTP0.8 branch working and then get PlotlyJS.jl and WebIO.jl working again 😅

So today, I plan to have a look at the branch HTTP0.8 and see if I can get it working. Have you done any addition work on that branch?

[EricForgy]

Running tests now. Wasn't too bad 😊🙏 (will update with edits to this comment)

• Update: Ok. Got several test failures. Will dig in now.
• Got some tests to pass 🎉
• Test are passing. Submitted a new PR https://github.com/JuliaWeb/WebSockets.jl/pull/142 😊

It turns out I just needed to set the rate_limit to a nonzero default value. I also made the name change back WSServer -> ServerWS.

I am keeping the name change WebsocketHandler -> WSHandlerFunction to be consistent with the new names RequestHandlerFunction and StreamHandlerFunction in HTTP.jl though.

I think we are close to being able to merge this to master and then we can help get some of the major dependencies, e.g. WebIO.jl, Mux.jl, PlotlyJS.jl, Blink.jl, etc updated.

What do you think? 🙏

[hustf]

Just merged #142 to branch HTTP0.8 and looking forward to test locally. I'm not satisfied with not accepting 0//1. We're generally thin on tests for server options, but that strays from the core functionality of this package so I suppose we should leave it at the level it is.

I sympatize with your struggle not having noted the other commits to branch, but having skimmed #137 again, I think I was pretty clear. When reading from the mailbox, I tend to miss the commit messages. Anyway, I promise to be very clear in the future and let's move on.

This work was also referred https://github.com/JuliaWeb/HTTP.jl/pull/393#issuecomment-472416293. I'll try and run the ad-hoc timeout tests locally as well.

How close we are to merging depends on how much time we're able to put in here and if new problems arise. But yes, of course, we're close 👍

[hustf]

Eric, sorry for the wall of text here. I need more time, but need to jot this down before next session. Comments appreciated, but it's mainly working notes.

1) As you can see, PR #143 CI tests are passing, as you have described. Congratulations, but I think we need to improve on quality yet.

2) test/timeout/timeout.jl tests are initially failing. This is due to a) the default rate_limit 0//1 not working. Local fix, test/timeout/timeout.jl:24: const SERVER = WebSockets.ServerWS(handle, gatekeeper, rate_limit = 1000//1) b) WebSockets.Response is no longer available. Local fix, test/timeout/timeout_functions.jl:35 handle(req) = read("limited life websockets.html") |> WebSockets.HTTP.Response Having fixed a) and b) locally, we hit the HTTP issue JuliaWeb/HTTP.jl#393 (comment). The first abnormal closure is pasted here:

In browser:
: 128s_timeout.onclose:wasClean: false; code: 1006; Abnormal Closure- reserved; reason: ;
  Websocket state is now 3 CLOSED.
In Julia console:
┌ Wslog 22:21:13.381: 256s_timeout was closed at  73 s  with message
└       WebSocketClosedError(" while read(ws|server) BoundsError(UInt8[], (1,))")
Inspecting the server .out channel (using `take!(SERVER.out)`. To get the stacktrace, repeat take!):
Base.IOError("stream is closed or unusable", 0)

 check_open at stream.jl:323 [inlined]                                                                                                                                                                                                                         
 uv_write_async(::Sockets.TCPSocket, ::Ptr{UInt8}, ::UInt64) at stream.jl:871                                                                                                                                                                                  
 uv_write(::Sockets.TCPSocket, ::Ptr{UInt8}, ::UInt64) at stream.jl:845                                                                                                                                                                                        
 unsafe_write(::Sockets.TCPSocket, ::Ptr{UInt8}, ::UInt64) at stream.jl:901                                                                                                                                                                                    
 unsafe_write at ConnectionPool.jl:132 [inlined]                                                                                                                                                                                                               
 macro expansion at gcutils.jl:87 [inlined]                                                                                                                                                                                                                    
 write at io.jl:165 [inlined]                                                                                                                                                                                                                                  
 writestartline(::HTTP.ConnectionPool.Transaction{Sockets.TCPSocket}, ::HTTP.Messages.Response) at Messages.jl:428                                                                                                                                             
 writeheaders(::HTTP.ConnectionPool.Transaction{Sockets.TCPSocket}, ::HTTP.Messages.Response) at Messages.jl:439                                                                                                                                               
 startwrite(::HTTP.Streams.Stream{HTTP.Messages.Request,HTTP.ConnectionPool.Transaction{Sockets.TCPSocket}}) at Streams.jl:85                                                                                                                                  
 handle(::HTTP.Handlers.RequestHandlerFunction{typeof(handle)}, ::HTTP.Streams.Stream{HTTP.Messages.Request,HTTP.ConnectionPool.Transaction{Sockets.TCPSocket}}) at Handlers.jl:273                                                                            
 (::getfield(WebSockets, Symbol("#_servercoroutine#7")){WebSockets.ServerWS})(::HTTP.Streams.Stream{HTTP.Messages.Request,HTTP.ConnectionPool.Transaction{Sockets.TCPSocket}}) at HTTP.jl:377                                                                  
 macro expansion at Servers.jl:352 [inlined]   

3) The imports from HTTP are removed. It was actually a conscious choice to be explicit doing those imports because a) Finding where those symbols are defined inside of HTTP has been a hassle due to the restructuring and the size of the package. b) More importantly, it enables dependants to refer two versions of HTTP simultaneously. WebSockets may be on the previous release, but I've had success locally with using two versions of HTTP simultaneously (just in different modules). E.g. moduleA: WebSockets.Response refers to julia\packages\HTTP\GN0Te\src\etc. module A.B Response would refer to F.julia\packages\HTTP\YjRCz\src\etc. You just need to be quite strict when importing.

I will revert this change by copying the commented imports from branch issue32.

4) The only ServerWS options that are actually passed now are: sslconfig, verbose and rate_limit. This is not your fault, I just discovered now that the arguments aren't splatted when passing this on.

Allowing the user to set options that are not used is just fooling people. Hence my earlier comments on simply sticking as close as possible to the new HTTP.jl interface. What to do?

[EricForgy]

Cool. No worries about my struggles. It's not your fault I don't pay close enough attention 😊

I do think it is good if we have one issue that stays open until all this is sorted and tagged 👍

1. As you can see, PR #143 CI tests are passing, as you have described. Congratulations, but I think we need to improve on quality yet.

Sure. Happy to help. Getting the tests to pass still feels like a nice milestone though. Despite all the (embarassingly large amount of) time I spent on that, in the end, I think the final changes were pretty minimal so shouldn't be too hard to wrap up things 🙏

2. test/timeout/timeout.jl tests are initially failing.

Feel free to push your local changes and I'll see if there is anything I can do to help.

3. The imports from HTTP are removed.

I think this was only in the tests, right?

It is fine to put them back if you prefer, but maybe we can think about it a bit. I think it is better practice to be explicit though (especially if overloading), but that is a style question. Personally, I would not import so much especially if the purpose is intended to support two different versions of HTTP 🤔

What if we respect the choice of HTTP and just do

using HTTP

What gets exported doesn't need to be explicit, but what isn't exported is still explicit? For example, I still like HTTP.Request and HTTP.Response 🤔

4. The only ServerWS options that are actually passed now are: sslconfig, verbose and rate_limit.

No worries. I think this should be an easy fix. I agree. We shouldn't offer an option that doesn't exist 😊

I confess that I was / am under extreme time pressure (as always) and jumped in trying to fix things without thinking too much about it. I did whatever I could to get things to work so I appreciate you taking the time to review and if we need to change anything, I'll do what I can to help.

[hustf]

samoconnor solved the timeout issue for us in https://github.com/JuliaWeb/HTTP.jl/pull/393. I implemented it. The timeout tests were a little confusing, as they put the various feedback into vectors. But the sequence in those vectors got messed up every time we hit the annoying and persisten "WARNING: Workqueue inconsistency detected: popfirst!(Workqueue).state != :queued".

To get the order of feedback consistent, the results are now put in a dictionary based on time_ns(). The timeout tests seem to pass fine now.

[hustf]

I discovered that e.g. @wslog sqrt(-2) did not output the error message as intended. This is really unrelated to this issue and could be merged to master. It just does not seem important enough to make a release by its own. Committed the fix and test for the fix to this branch instead. Ref. commits f623e33 and 2cded74

[hustf]

We discussed earlier the closing method. You removed the asyncronous task in serve that waits for something on the .in channel. You also added a 'Base.close' method.

We should have added the Base.close method long ago, and of course we'll keep it. But just now it does not work satisfactorily: We replace https://github.com/JuliaWeb/WebSockets.jl/blob/master/examples/minimal_server.jl lines 47-49 with the following and get:

julia> @async begin
       sleep(40)
       close(server)
       end
Task (runnable) @0x0000000008944010

julia> WebSockets.serve(server, LOCALIP, PORT)
┌ Warning: throttling 192.168.0.4
└ @ HTTP.Servers C:\Users\F\.julia\packages\HTTP\GN0Te\src\Servers.jl:54
ERROR: IOError: accept: software caused connection abort (ECONNABORTED)

Aside from the throttling, which we'll fix, the dirty exit is moreover ugly bugly. I'll reinstate the old method, and use it in base.close.

[hustf]

Reinstated the ServerWS.in closing method in commit 7fe8c77. But we also need to change back the type of the server field to be a Ref{Union{...}} for this to work properly. But we'll do that when restructuring the types (coming soon).

[hustf]

Still postponing the structural changes here, since it's nice to have everything working fully first.

The only 'structural' change so far is not referring tcpserver in ServerWS (c643303). I think it's crucial to be able to drop that reference when closing, which is why we're again running garbage collection after setting the reference to nothing. It should not be necessary, but I have a limited understanding of the interaction between libuv, Julia and garbage collection. The reference also just lives in the 'serve' thread, which exits cleanly.

After commit c643303 and aa3e207, PR #143, all symbols are imported and not referred directly. This includes stdlib imports from Base, Base64, Sockets. I don't really know if this helps compiling, and I'm not sure if it triggers more errors at compile-time, as oposed to during tests. But I have that feeling. We also drop the direct dependency on MBedTLS, as that could easily be the latest version when HTTP has moved on to the next round of changes. I believe having different versions of MBedTLS referred simultaneously can be quite troublesome (just switching between Julia versions can trigger rebuilding).

[hustf]

I won't finish this today it seems, but I wanted to end up with this (HTTP.jl:411):

    HTTP.listen(_servercoroutine,
            host, port,
            verbose = verbose,
            rate_limit = serverws.rate_limit,
            sslconfig = serverws.sslconfig,
            tcpisvalid = serverws.tcpisvalid,
            server = tcpserver[],
            reuseaddr = serverws.reuseaddr,
            connection_count = serverws.connection_count,
            rate_limit = serverws.rate_limit,
            reuse_limit = serverws.reuse_limit,
            readtimeout = serverws.readtimeout)

I.e. dropping ServerOptions.

ServerWS will have many fields, but since we now have our very own 'show' method, there's no need to output default server options all over the place.

[hustf]

It is also tempting to put in another modernization here, in line with what HTTP 0.8 did. You have introduced a WSHandlerFunction type. That is really elegant. How about having two types? One which takes the socket as an argument, and another which also takes the request for security. We could then drop 'isapplicable', which may possibly slow things down. Type inference is more 'Julian' anyway.

I am kind of hoping establishing websockets will take no time at all in the future. It would be nice to have a websocket for lots of different objects on a page, without slow-down. At this point, I would go for using just a few simultaneous sockets from each page, which adds a lot of complexity and code lines in Ecmascript.

[hustf]

@EricForgy ,I had to think about your comments above for a while. Now I have all the code 'in my head' again.

test/timeout/timeout.jl tests are initially failing. Feel free to push your local changes and I'll see if there is anything I can do to help.

The changes weren't originally pushed, because parts of it dealt with the missing imports. I hope you'll give it a try now, everything's working fine.

The imports from HTTP are removed. I think this was only in the tests, right?

No, the list was pretty extensive. I pulled in commented imports from branch issue130 (which we otherwise dropped since you made this great effort).

It is fine to put them back if you prefer, but maybe we can think about it a bit. I think it is better practice to be explicit though (especially if overloading), but that is a style question. Personally, I would not import so much especially if the purpose is intended to support two different versions of HTTP 🤔

What if we respect the choice of HTTP and just do using HTTP

While reading just parts of these long files, your approach is no doubt better.

Still, I think both styles can be called explicit. We certainly want to be explicit. But with something like 35 imported symbols (this includes Base, Base64, Socket), the overview we get from imports will also be of help.

Julia has become quite the formalistic language now, but perhaps this is the only way to build large dependency trees? It certainly hits my productivity with the language for declarative/ scripting work. We were quite informal in just splatting server options as keywords and hope they get picked up by HTTP down the line. At this point, I would prefer not accepting arguments we don't know about.

What gets exported doesn't need to be explicit, but what isn't exported is still explicit? For example, I still like HTTP.Request and HTTP.Response 🤔

I don't think I quite understand what you mean here. But regarding exports, HTTP is very conservative and exports little. When we make types like ServerWS, we can export it with few concerns. When in doubt, don't change what is exported. The current list of exports is identical to what we had before, meaning that examples and tests could work with a minimum of change.

I confess that I was / am under extreme time pressure (as always) and jumped in trying to fix things without thinking too much about it. I did whatever I could to get things to work so I appreciate you taking the time to review and if we need to change anything, I'll do what I can to help.

I don't know if this translates at all, but it helps to have a smooth back when there's time pressure. We need to continue having fun coding, and it's more fun without bugs.

[hustf]

This went pretty smoothly actually. No tests were changed and locally they pass fine.

The 'normal' interface is totally unchanged, but one can now pass all arguments as named keywords, ServerWS can be inspected for the total number of connections.

Commit message from 010eb74 :

modified: src/HTTP.jl

• Remove ServerOptions, expand as more fields in ServerWS. Defaults values in DEFAULTOPTIONS::NamedTuple.
• Remove the field .logger. The modern approach is to use Logging.with_logger
• Remove keyword arguments 'cert' and 'key'. The user must pass sslconfig = SSLConfig(cert, key)
• No longer an option to pass ratelimits. HTTP.listenloop constructs that internally.
• ServerWS is now immutable (but fields like connection_count is not)
• Inline docs and comments not fully updated

modified: src/show_ws.jl

• show method for the new ServerWS type. Looks identical when no special options are set.

modified: test/show_test.jl

• Modify expected output ServerWS.
• Drop tests with the logger option. The replacement functionality is already covered in test/test_websocketlogger.jl
• Have not added test for ServerWS.connection_count

[hustf]

Updated 'Errors after updating?' and inline docs in commit 4bf760c. Hoping to make a merge to master and release tomorrow, but should check that examples are still working first.

[hustf]

I take the emoticons to say you're OK with a quick merge and release now.

Updating the 'listen-syntax' examples show the benefit of the WebSockets.serve(ServerWS) syntax: Practically no changes vs. many changes.

I put the changes into README.md. There's no deprecation cycle with warnings: That's simply too much work and there's more changes down the road anyway.

Also tried to get my head around the HTTP package. The routing stuff is pretty complicated, and the interactive utilties don't help much in following the function calls.

It would be great to have minimal routing examples here, though. Pulling in Mux.jl examples here is sort of going the wrong way in the dependency tree, but having examples of how to use the underlying functionality in HTTP is another matter.

Commit 1de646d message:

Brush-up on examples and README, also add test

modified:   README.md
            More transition to 1.5 tips.
modified:   examples/client_repl_input.jl
            Add hint to use minimal_server.jl
deleted:    examples/minimal_server_listen_do_syntax.jl
            Do syntax no longer supported; must wrap handler.
modified:   examples/minimal_server_listen_syntax.jl
            Made asyncronous, updated syntax.
modified:   src/HTTP.jl
            Drop inline listen syntax example
modified:   src/show_ws.jl
            Fixed error when connection_count > 0
modified:   test/show_test.jl
            Test connection_count > 0

[hustf]

@eric , I was just about tagging a new version from master, but I think you should do a quick check of Manifest.toml and Project.toml. I don't look at those files much, since I have been under the impression that a REQUIRE file is all that is actually needed. I'm worried that there might be some additional requirements in the .toml file? If so, it'll cause issues for somebody.

Since these files were included, we have dropped MbedTLS as a direct dependency in REQUIRE.

[hustf]

Thanks for input, Eric. Closing this now.