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

with_logger(NullLogger()) breaks MeshCat #68

Closed
tkoolen opened this issue Nov 7, 2018 · 7 comments · Fixed by #141
Closed

with_logger(NullLogger()) breaks MeshCat #68

tkoolen opened this issue Nov 7, 2018 · 7 comments · Fixed by #141

Comments

@tkoolen
Copy link
Collaborator

tkoolen commented Nov 7, 2018

I wanted to make MeshCat/the web stack a little less verbose, so I tried using with_logger(NullLogger()):

julia> using MeshCat

julia> vis = Visualizer()
MeshCat Visualizer with path /meshcat

julia> using Logging

julia> with_logger(NullLogger()) do
           open(vis)
       end
Process(`xdg-open http://127.0.0.1:8700`, ProcessExited(0))

Unfortunately, only the first time I run open(vis), I get

image

If I reload the browser tab, it's fine. Should open include a wait or something?

@rdeits
Copy link
Owner

rdeits commented Nov 9, 2018

Uh, fascinating. Yeah, I think this is just a symptom of some broader issues with deciding when to be synchronous vs. asynchronous. I'll take a look at this issue specifically, though.

@tkoolen
Copy link
Collaborator Author

tkoolen commented Nov 20, 2018

Alright, so

@async WebIO.webio_serve(Mux.page("/", req -> core.scope), port)

calls WebIO.webio_serve, which calls Mux.serve, which is @async anyway, so I think the @async in open isn't necessary. But the question still remains how to assure that the web server is up and running before calling open_url. If we could just create the HTTP.Server ourselves, we could probably do that using isopen(::Server), right?

@rdeits
Copy link
Owner

rdeits commented Nov 21, 2018

I think we might still need the @async, because webio_serve actually calls this Mux.serve method: https://github.com/JuliaWeb/Mux.jl/blob/849d72c245f0ba32c8f856b27a393d5aa7b99e5b/src/server.jl#L65 which in turn calls https://github.com/JuliaWeb/WebSockets.jl/blob/0aaca05824df7a02c5feaca910453a63083877c0/src/HTTP.jl#L359 whish is not async.

I wonder if we could copy that WebSockets.serve() method into MeshCat, but make the HTTP.listen call async and return the tcpserver.

@rdeits
Copy link
Owner

rdeits commented Nov 21, 2018

This seems to work, and might be a suitable replacement for webio_serve (while also returning the underlying TCPServer. It's basically just a combination of the relevant webio_serve, Mux.serve and WebSockets.serve methods, with a bit of cleanup:

using WebIO
using WebSockets
using Mux
using HTTP
using Sockets

app = Mux.page("/", req -> "hello")

http = Mux.App(Mux.mux(Mux.defaults, app, Mux.notfound()))
websock = Mux.App(Mux.mux(
    Mux.wdefaults,
    Mux.route("/webio-socket", WebIO.create_socket),
    Mux.wclose,
    Mux.notfound(),
))

server = WebSockets.ServerWS(Mux.http_handler(http), Mux.ws_handler(websock))
host = Mux.localhost

function serve(server::WebSockets.ServerWS{T, H, W}, host, port, verbose) where {T, H, W}
    
    tcpserver = Ref(Sockets.listen(Sockets.InetAddr(host, port)))
    
    @async begin
        take!(server.in)
        close(tcpserver[])
    end
    
    function _servercoroutine(stream::HTTP.Stream)
        try
            if WebSockets.is_upgrade(stream.message)
                WebSockets.upgrade(server.wshandler.func, stream)
            else
                WebSockets.handle_request(server.handler.func, stream)
            end
        catch err
            put!(server.out, err)
            put!(server.out, stacktrace(catch_backtrace()))
        end
    end
    
    @async HTTP.listen(_servercoroutine,
            host, port;
            tcpref=tcpserver,
            ssl=(T == HTTP.Servers.https),
            sslconfig = server.options.sslconfig,
            verbose = verbose,
            tcpisvalid = server.options.ratelimit > 0 ? checkratelimit! :
                                                     (tcp; kw...) -> true,
            ratelimits = Dict{IPAddr, HTTP.Servers.RateLimit}(),
            ratelimit = server.options.ratelimit)
    return tcpserver[]
end

port = 8004
tcpserver = serve(server, host, port, false)
run(`xdg-open http://localhost:$port`)

@tkoolen
Copy link
Collaborator Author

tkoolen commented Nov 21, 2018

Ah right. I was assuming that the WebIO.serve methods all funnel into one.

I'm a little bit worried about this being brittle; it's quite a bit of additional code that we need to keep in sync with the WebIO stack. But maybe that's just what's needed. Another option could be to open a few PRs against various web stack packages (man there's a lot of them).

Does this really work by the way? Since the tcpserver Ref is assigned to in the @async listen call, isn't there a chance that it's still Ref(nothing) when you run xdg-open?

I was wondering if we could just use the first argument to webio_serve to somehow set up a Channel that we can listen to, but I think the relevant callable is only called upon a request.

@rdeits
Copy link
Owner

rdeits commented Nov 21, 2018

Yeah, I'd prefer to get this into WebSockets instead of having to maintain it ourselves, but we can at least try it out and see if it works.

I think this should work, although we might need to wait until isopen(tcpserver) == true. There's no danger of it being nothing in the implementation I pasted above: I'm constructing and assigning the ref all at once, while the implementation in WebSockets.jl did leave it unassigned until the call to listen. As far as I can tell, the only reason that's done is to allow reusing an existing server or port, which isn't something we need to do here.

@tkoolen
Copy link
Collaborator Author

tkoolen commented Nov 21, 2018

I'm constructing and assigning the ref all at once.

Oh yeah, I hadn't noticed you'd changed that.

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

Successfully merging a pull request may close this issue.

2 participants