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.

Sign up for GitHub

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

Jump to bottom

revise(throw=true) seems to not raise an exception #541

Closed
clarkevans opened this issue Sep 23, 2020 · 4 comments · Fixed by #548
Closed

revise(throw=true) seems to not raise an exception #541

clarkevans opened this issue Sep 23, 2020 · 4 comments · Fixed by #548

Comments

@clarkevans
Copy link

clarkevans commented Sep 23, 2020 •
edited
Loading

With the help of Mark Kittisopikul & Tim Larson on Slack, I got a simple HTTP.jl use with Revise.jl v2.7.6 to work on Julia v1.5.1. When I update my function and press refresh on the browser, I get a properly updated page. Following is the example, hello.jl which can be run with using Revise; includet("hello.jl"); serve() and used by pointing your browser to https://127.0.0.1:8080. I could, for example add exclamation marks to Hello World, save "hello.jl", and when I refresh the browser, it properly updates the display, printing "Revise worked...." on the console.

However, if you make an error in this file, say by adding bug on line #2, it seems that Revise.revise(; throw=true) has interesting behavior: it prints out the failure to its log, but then neglects to throw the error. Indeed, with the code below it still prints "Revise worked..." to the console after printing out the error. You could see this in the source code, at https://github.com/timholy/Revise.jl/blob/v2.7.6/src/Revise.jl#L814-L825 which prints the error... but doesn't seem to throw an exception.

Following is the hello.jl file... and the console log.

using HTTP

homepage(req::HTTP.Request) =
    HTTP.Response(200, "<html><body>Hello World!</body></html>")

function revise_function(func)
    (args...; kw...) -> begin
        try
            Revise.revise(; throw = true)
            println("Revise worked....")
        catch x
            println("Revise failed....")
            Revise.errors()
            return HTTP.Response(500, "There was a revision error")
        end
        Base.invokelatest(func, args...; kw...)
    end
end

const ROUTER = HTTP.Router()
HTTP.@register(ROUTER, "GET", "/", revise_function(homepage))

serve() = HTTP.serve(ROUTER, "127.0.0.1", 8080)

The console log...

julia> using Revise; includet("hello.jl"); serve();
Revise worked....
Revise worked....
Revise.LogRecord(Error, evaluation error starting at /home/cce/TodoMVC.jl/src/hello.jl:2, lowered, Revise_e943ed8b, "/home/cce/.julia/packages/Revise/qxX5H/src/lowered.jl", 106, (mod=Main, ex=begin
    #= /home/cce/TodoMVC.jl/src/hello.jl:2 =#
    bug
    #= /home/cce/TodoMVC.jl/src/hello.jl:3 =#
    homepage(req::HTTP.Request) = begin
            #= /home/cce/TodoMVC.jl/src/hello.jl:3 =#
            HTTP.Response(200, "<html><body>Hello World!</body></html>")
        end
end)UndefVarError: bug not defined
Stacktrace:
 [1] step_expr!(::Any, ::JuliaInterpreter.Frame, ::Any, ::Bool) at /home/cce/.julia/packages/JuliaInterpreter/CPmYX/src/interpret.jl:548)
Revise worked....
┌ Error: error handling request
│   exception =
│    MethodError: no method matching homepage(::HTTP.Messages.Request)
│    Stacktrace:
│     [1] #invokelatest#1 at ./essentials.jl:710 [inlined]
│     [2] invokelatest at ./essentials.jl:709 [inlined]
│     [3] (::var"#1#3"{var"#1#2#4"{typeof(homepage)}})(::HTTP.Messages.Request; kw::Base.Iterators.Pairs{Union{},Union{},Tuple{},NamedTuple{(),Tuple{}}}) at /home/cce/TodoMVC.jl/src/hello.jl:16
│     [4] #1 at /home/cce/TodoMVC.jl/src/hello.jl:8 [inlined]
│     [5] handle at /home/cce/.julia/packages/HTTP/IAI92/src/Handlers.jl:253 [inlined]
│     [6] handle(::HTTP.Handlers.RequestHandlerFunction{var"#1#3"{var"#1#2#4"{typeof(homepage)}}}, ::HTTP.Streams.Stream{HTTP.Messages.Request,HTTP.ConnectionPool.Transaction{Sockets.TCPSocket}}) at /home/cce/.julia/packages/HTTP/IAI92/src/Handlers.jl:276
│     [7] handle(::HTTP.Handlers.Router{Symbol("##254")}, ::HTTP.Streams.Stream{HTTP.Messages.Request,HTTP.ConnectionPool.Transaction{Sockets.TCPSocket}}) at /home/cce/.julia/packages/HTTP/IAI92/src/Handlers.jl:466
│     [8] #4 at /home/cce/.julia/packages/HTTP/IAI92/src/Handlers.jl:345 [inlined]
│     [9] macro expansion at /home/cce/.julia/packages/HTTP/IAI92/src/Servers.jl:367 [inlined]
│     [10] (::HTTP.Servers.var"#13#14"{HTTP.Handlers.var"#4#5"{HTTP.Handlers.Router{Symbol("##254")}},HTTP.ConnectionPool.Transaction{Sockets.TCPSocket},HTTP.Streams.Stream{HTTP.Messages.Request,HTTP.ConnectionPool.Transaction{Sockets.TCPSocket}}})() at ./task.jl:356
└ @ HTTP.Servers ~/.julia/packages/HTTP/IAI92/src/Servers.jl:373
@timholy
Copy link
Owner

timholy commented Sep 23, 2020

I haven't looked at the issue you're reporting, but FYI you might be a better candidate for entr.

But I will look at this tomorrow, thanks for reporting.

@clarkevans
Copy link
Author

I posted this as a documentation issue (JuliaWeb/HTTP.jl#587) for HTTP.jl. In that ticket is an implementation using entr. Even so, one still needs a way to detect that the update failed due to an error, and then, once that error is fixed, resume serving. I'm not sure what that logic should look like, but this exception based logic doesn't work (either bug or by design).

@timholy
Copy link
Owner

timholy commented Sep 24, 2020

Oh, interesting. It looks like the throw kwarg only gets used by process_user_callbacks. Do you need it to run from revise?

I posted a hint at JuliaWeb/HTTP.jl#587 (comment). I'd guess if you change it, then it will work as you expect. If there's an error partway through the file that's the source of the entr command, I am not sure what would happen.

@clarkevans
Copy link
Author

clarkevans commented Sep 24, 2020 •
edited
Loading

Well, based upon documentation, I was expecting the code fragment above to work, as I need to return 5xx if the changed source files have syntax errors (it'd be great to even show the lines of the errors...). I still think it makes sense for throw to make revise raise an exception if it encountered any difficulty... but I'm a naive user. This is far more complex than I expected. Generally, I'd prefer the web server to continue to fail so long as there are any outstanding errors. Hence, if there are any project errors, I'd love if revise(;throw=true) raised an exception. Bonus points if it gave me the list of the source files, changes with issues. I'm trying to enable a web-developer workflow, where the browser serves as the REPL.

timholy added a commit that referenced this issue Oct 3, 2020
@timholy
revise should throw when throw=true
2622ccb 
Fixes #541
@timholy timholy mentioned this issue Oct 3, 2020
Merged
timholy added a commit that referenced this issue Oct 4, 2020
@timholy
revise should throw when throw=true
e46039c 
Fixes #541
timholy added a commit that referenced this issue Oct 4, 2020
@timholy
revise should throw when throw=true (#548)
cc3f822 
Fixes #541
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

revise should throw when throw=true timholy/Revise.jl
2 participants
@clarkevans @timholy