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

Make sleep(n) and Timer creation more MT-safe #32174

Merged
merged 7 commits into from
Jun 4, 2019
+21 −14
Merged

Make sleep(n) and Timer creation more MT-safe #32174

Changes from 1 commit
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
6f0088d
Make sleep(n) and Timer more MT-safe
jpsamaroo May 28, 2019
3d8312a
Include uv_timer_init and jl_uv_associate_julia_struct
jpsamaroo May 29, 2019
6fd062f
Add back handling of err != 0
jpsamaroo May 29, 2019
0596430
Add casts, fix return type
jpsamaroo May 29, 2019
f2a2152
Address feedback from @vtjnash
jpsamaroo May 29, 2019
7afbd53
Change Ptr{Cvoid} to Any in ccall
jpsamaroo May 30, 2019
e85f00d
Remove checking of error codes
jpsamaroo Jun 1, 2019
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
Prev Previous commit
Next Next commit
Add casts, fix return type
@jpsamaroo
jpsamaroo committed May 29, 2019
commit 05964305b225a2b639c8e3f074c639198b222920
4 changes: 2 additions & 2 deletions src/jl_uv.c
View file
Original file line number Diff line number Diff line change
@@ -1094,10 +1094,10 @@ JL_DLLEXPORT int jl_uv_update_timer_start(uv_loop_t* loop, uv_timer_t* handle,
if (err) {
Copy link
Member

Choose a reason for hiding this comment

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

this actually can't fail, so we can just delete the error checking code (or make it an abort)

// TODO: this codepath is currently not tested
free(handle);
Copy link
Member

Choose a reason for hiding this comment

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

missing JL_UV_UNLOCK on this codepath, also probably better to keep the free call in Julia

Copy link
Member Author

Choose a reason for hiding this comment

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

Hopefully my latest commit addresses both of these properly 😄

return NULL;
return err;
}

jl_uv_associate_julia_struct(handle, handle);
jl_uv_associate_julia_struct((uv_handle_t *)handle, (jl_value_t *)handle);
Copy link
Member

Choose a reason for hiding this comment

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

one of these things is not like the other—you can't cast to both libuv and julia

Copy link
Member Author

Choose a reason for hiding this comment

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

Sorry, I'm getting slightly confused by what's what, since in the following two locations it seems that we're already passing objects which (to my uneducated eyes) seem to end up with casts that are essentially the same as what you pointed out at:

julia/base/asyncevent.jl

Line 90 in 4b0c8e7

err = ccall(:uv_timer_init, Cint, (Ptr{Cvoid}, Ptr{Cvoid}), eventloop(), this)

julia/base/asyncevent.jl

Lines 102 to 103 in 4b0c8e7

ccall(:uv_timer_start, Cint, (Ptr{Cvoid}, Ptr{Cvoid}, UInt64, UInt64),
this, uv_jl_timercb::Ptr{Cvoid},

Those two libuv calls don't expect a jl_value_t*, even though that's what we're doing there, right? I assume it works because this.handle is the first field in the Timer struct so the pointers are the same anyway?

uv_update_time(loop);
int r = uv_timer_start(handle, cb, timeout, repeat);
JL_UV_UNLOCK();