Skip to content

Fix some JET errors around matching methods for send_connection_hdr(...) and send_msg_now(...) (underlying problem: the Worker(...) constructors might not always return a Worker) #180

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

Merged
DilumAluthge merged 2 commits into from
Feb 21, 2026
Merged

Fix some JET errors around matching methods for send_connection_hdr(...) and send_msg_now(...) (underlying problem: the Worker(...) constructors might not always return a Worker) #180

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
a18f0b9
Fix some JET errors around matching methods for `send_connection_hdr(…
DilumAluthge Feb 20, 2026
5cf8ca4
Merge branch 'master' into dpa/fix-jet-send_connection_hdr
DilumAluthge Feb 21, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/cluster.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -626,7 +626,7 @@ function create_worker(manager::ClusterManager, wconfig::WorkerConfig)
end
end

w = Worker(w_stub.id, r_s, w_s, manager; config=wconfig)
w = Worker(w_stub.id, r_s, w_s, manager; config=wconfig)::Worker
Copy link
Member

@JamesWrigley JamesWrigley Feb 20, 2026

Choose a reason for hiding this comment

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

Would this also be fixed by adding ::Worker return annotations to the constructors?

Copy link
Member Author

@DilumAluthge DilumAluthge Feb 21, 2026

Choose a reason for hiding this comment

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

Hmmm. I think we need to look at this code closer.

The problematic line seems to be line 143 here:

Distributed.jl/src/cluster.jl

Lines 140 to 152 in d06aa73

function Worker(id::Int, conn_func)
@assert id > 0
if haskey(map_pid_wrkr, id)
return map_pid_wrkr[id]
end
w=new(id, Threads.ReentrantLock(), [], [], false, W_CREATED, Threads.Condition(), time(), conn_func)
w.initialized = Event()
register_worker(w)
w
end
Worker() = Worker(get_next_pid())
end

Line 143 is this line:

return map_pid_wrkr[id]

map_pid_wrkr is a global constant. The problem (I think) is the type of map_pid_wrkr:

Distributed.jl/src/cluster.jl

Line 850 in d06aa73

const map_pid_wrkr = Dict{Int, Union{Worker, LocalProcess}}()

So when we index into map_pid_wrkr with map_pid_wrkr[id], how do we know whether map_pid_wrkr[id] is a Worker or a LocalProcess?

Copy link
Member

@JamesWrigley JamesWrigley Feb 21, 2026

Choose a reason for hiding this comment

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

Wow, I absolutely hate that 🤮 But something to fix another time... We could safely add an annotation to this constructor though, which I think is the one getting called here:

Distributed.jl/src/cluster.jl

Line 124 in d06aa73

function Worker(id::Int, r_stream::IO, w_stream::IO, manager::ClusterManager;

Copy link
Member Author

@DilumAluthge DilumAluthge Feb 21, 2026

Choose a reason for hiding this comment

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

I tried adding ::Worker to that constructor (the constructor starting on line 124), but that's not sufficient. Here's the source code for that constructor:

Distributed.jl/src/cluster.jl

Lines 124 to 137 in 2adcd26

function Worker(id::Int, r_stream::IO, w_stream::IO, manager::ClusterManager;
version::Union{VersionNumber, Nothing}=nothing,
config::WorkerConfig=WorkerConfig())
w = Worker(id)
w.r_stream = r_stream
w.w_stream = buffer_writes(w_stream)
w.w_serializer = ClusterSerializer(w.w_stream)
w.manager = manager
w.config = config
w.version = version
set_worker_state(w, W_CONNECTED)
register_worker_streams(w)
w
end

The problem is, on line 127, that constructor calls the Worker(::Int) constructor, which in turn calls the Worker(::Int, ::Any) constructor, and now we're in the Worker(::Int, ::Any) constructor, which is the constructor that includes the return map_pid_wrkr[id] line.

So, if I try adding ::Worker to the line 124 constructor, JET still complains, because Worker(::Int, ::Any) might return a LocalProcess.

Copy link
Member

@JamesWrigley JamesWrigley Feb 21, 2026

Choose a reason for hiding this comment

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

Bleh, devastating.

Copy link
Member Author

@DilumAluthge DilumAluthge Feb 21, 2026

Choose a reason for hiding this comment

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

I've opened an issue: #182

# install a finalizer to perform cleanup if necessary
finalizer(w) do w
if myid() == 1
Expand Down