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

Fix #231 #232

Merged
merged 1 commit into from
Nov 18, 2024
Merged

Conversation

frankier
Copy link
Contributor

@frankier frankier commented Nov 5, 2024

Fixes #231

This fixes the problem which is that a error when Revise is used together with a middleware vector which isn't Any[] (note that a single element array will always be typed).

I've also added some tests which ended up being a bit simpler than I thought.

@frankier frankier changed the title Revise fix and tests Fix #231 and add Revise tests Nov 5, 2024
@@ -132,6 +132,7 @@ function serve(ctx::Context;
if ctx.mod === nothing
@warn "You are trying to use the `revise` option without @oxidise. Code in the `Main` module, which likely includes your routes, will not be tracked and revised."
end
middleware = convert(Vector{Any}, middleware)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

rather than performing a type conversion on the entire array, could we try setting the middleware keyword type to Vector{Function}. This should be generic enough to handle any weird edge cases

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think doing that will mean that middleware can't be implemented as functors/callable structs. Since Julia doesn't have interfaces/traits there's no way to get the type Union[Callable Structs, Function}.

e.g.

function testy(f::Vector{Function})
    for i in f
        i(1)
    end
end

struct MyFunctor end

function (f::MyFunctor)(x)
    return x + 1
end

testy([MyFunctor()])

ERROR: MethodError: no method matching testy(::Vector{MyFunctor})

Dispatching on a more general parameter won't work since Julia containers as type invariant:

function testy2(f::Vector{Any})
    for i in f
        i(1)
    end
end

testy2([MyFunctor()])

ERROR: MethodError: no method matching testy2(::Vector{MyFunctor})

The latter could be made to work, but it means that users have to pass middleware as: middleware=Any[mymiddleware1] if there is a single middleware.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@frankier,

I think that's actually more of a feature than a limiting factor. The underlying implementation by HTTP only accepts higher ordered functions as valid middleware. While technically functors could be used for middleware - I'm not sure I'd want to enable that.

What scenarios do you see this being more helpful than a regular HOF?

Copy link
Contributor Author

@frankier frankier Nov 14, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When making a functor (C++ term) or callable (Python term) the idea is to override the "function-pointer call" operator, so that the struct behaves like a function pointer. In practice it can be useful e.g. when you want to have multiple initialisation paths. Another possibility is that you may want to add extra methods to the middleware if it exists as part of a larger flow.

It is true that things like this are not strictly necessary. One can always wrap ones callable in a HOF, but then why should one have to? Accepting both functions and HOF is part of conventional Julia interfaces.

It currently works totally fine with both HTTP.jl and Oxygen.jl, e.g. I have been using the following middleware for profiling:

using Profile
using ProfileSVG
using Oxygen
using HTTP


@kwdef mutable struct FlamegraphMiddleware
    profidx::Int = 1
    lock::ReentrantLock = ReentrantLock()
    profilesvg_kwargs=(;)
end


function (middleware::FlamegraphMiddleware)(handler)
    return function(req::HTTP.Request)
        Profile.clear()
        try
            return @profile handler(req)
        finally
            mkpath("profiles")
            data = Profile.fetch()
            if isempty(data)
                @info "No profile data (try increasing the sample rate)"
            else
                Profile.clear()
                local dest
                lock(middleware.lock)
                try
                    dest = joinpath("profiles", "$(middleware.profidx).svg")
                    middleware.profidx += 1
                finally
                    unlock(middleware.lock)
                end
                rm(dest; force=true)
                ProfileSVG.save(dest, data; middleware.profilesvg_kwargs...)
                @info "Profile saved to $dest with $(length(data)) samples"
            end
        end
    end
end

I think its nice that both HOF and struct middleware work fine.

Types in function signatures are usually mainly for dispatch in Julia. If you pass something of the wrong type, you'll just get a MethodError, and Julia specializes at call-time. I think functions are traditionally an exception to this and you have to specifically add in an unbounded type variable for specialisation. See https://docs.julialang.org/en/v1/manual/performance-tips/#Be-aware-of-when-Julia-avoids-specializing

ETA: Hmm, it looks like my example isn't quite right. I guess I misremembered and I was actually doing a two-phase initialization there. I'm in the habit of using functors over HOF but maybe you're right about HTTP.jl let me get back in a moment.

Copy link
Contributor Author

@frankier frankier Nov 14, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It does seem that HTTP.jl is duck-typed too

https://github.com/JuliaWeb/HTTP.jl/blob/b7f5aa8ede99db7139bdbbe6e430c6420c0d8795/src/Handlers.jl#L346-L351

So although my example wasn't right, I stand by the idea that allowing either is the most conventional style in Julia. Julia could get better type-checking could happen after interfaces are introduced and JET supports them, but at the moment JET is too strict for most real code. So ironically(?) Typescript and Python+mypy have better typechecking than Julia at the moment.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, in that case it makes sense to keep it open if HTTP allows it

@frankier frankier force-pushed the revise-fixes-and-tests branch from 6576e9d to 2ef3de1 Compare November 6, 2024 10:43
@frankier frankier changed the title Fix #231 and add Revise tests Fix #231 Nov 6, 2024
@frankier
Copy link
Contributor Author

frankier commented Nov 6, 2024

I've moved the tests to #233

@frankier frankier requested a review from ndortega November 11, 2024 06:40
@ndortega ndortega merged commit 94af0f7 into OxygenFramework:master Nov 18, 2024
6 of 7 checks passed
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 this pull request may close these issues.

Incompatibility between revise and middleware arguments in serve?
2 participants