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

Re-enable SIGINT handler in pyjlwrap_call #574

Open
wants to merge 7 commits into
base: master
Choose a base branch
from
Open

Re-enable SIGINT handler in pyjlwrap_call #574

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
0d85d79
Re-enable SIGINT handler in pyjlwrap_call
tkf Sep 19, 2018
bd0fcd8
Wrap more ccall with disable_sigint
tkf Sep 19, 2018
7bdfe5f
Revert "Wrap more ccall with disable_sigint"
tkf Sep 20, 2018
217ebd0
Replace almost all ccall with ccall macro
tkf Sep 20, 2018
0b810b5
Add pyccall macro to combine ccall and pysym macros
tkf Sep 20, 2018
f2484ef
Remove unused imports
tkf Sep 24, 2018
ec5d1cb
Add some comments for ccall macro
tkf Sep 24, 2018
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
78 changes: 46 additions & 32 deletions src/PyCall.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,11 +20,6 @@ import Base: size, ndims, similar, copy, getindex, setindex!, stride,
append!, insert!, prepend!, unsafe_convert
import Compat: pushfirst!, popfirst!, firstindex, lastindex

# Python C API is not interrupt-safe. In principle, we should
# use sigatomic for every ccall to the Python library, but this
# should really be fixed in Julia (#2622). However, we will
# use the sigatomic_begin/end functions to protect pycall and
# similar long-running (or potentially long-running) code.
import Base: sigatomic_begin, sigatomic_end

import Conda
Expand All @@ -36,6 +31,25 @@ import Base.Iterators: filter
include(joinpath(dirname(@__FILE__), "..", "deps","depsutils.jl"))
include("startup.jl")

# Python C API is not interrupt-safe. In principle, we should
# use sigatomic for every ccall to the Python library, but this
# should really be fixed in Julia (#2622). However, we will
# use the `disable_sigint` function to protect *all* invocations
# of Python API. This is required since `pyjlwrap_call` uses
# `reenable_sigint` which has to be called within `disable_sigint`
# context.
macro ccall(args...)
quote
disable_sigint() do
Copy link
Member

Choose a reason for hiding this comment

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

This adds a higher-order function call to every ccall; have you checked whether it makes a measurable difference in performance? (Hopefully it all gets inlined, but…)

I really wish this would get fixed in Julia itself.

Copy link
Member

Choose a reason for hiding this comment

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

Wait, JuliaLang/julia#2622 is closed, does that mean we don't need this anymore?

Copy link
Member

Choose a reason for hiding this comment

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

Oh, now I vaguely remember JuliaLang/julia#16174 — we still need this for any ccall in that might call back to Julia code in which we want to catch interrupt exceptions, I think?

Copy link
Member

Choose a reason for hiding this comment

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

The comment should clarify this, in any case.

Copy link
Member Author

Choose a reason for hiding this comment

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

You mean a comment like this?:

Although #2622 is closed, ccall still does not automatically disable SIGINT.

have you checked whether it makes a measurable difference in performance?

No, but the code without it was unsafe. It manifested easily because I added reenable_sigint so that julia runtime complains. Wasn't it needed to avoid setfault like #15?

Having said that, there are places where we can merge disable_sigint and do it in a batch while using ccalls. If we detect performance regression I guess we can use such tricks. I need to look at benchmarks/ code to do the check.

ccall($(esc.(args)...))
end
end
end

macro pyccall(f, args...)
:(@ccall(@pysym($(esc(f))), $(esc.(args)...)))
end

#########################################################################

# Mirror of C PyObject struct (for non-debugging Python builds).
Expand Down Expand Up @@ -99,7 +113,7 @@ it is equivalent to a `PyNULL()` object.
ispynull(o::PyObject) = o.o == PyPtr_NULL

function pydecref_(o::Union{PyPtr,PyObject})
ccall(@pysym(:Py_DecRef), Cvoid, (PyPtr,), o)
@pyccall(:Py_DecRef, Cvoid, (PyPtr,), o)
return o
end

Expand All @@ -110,7 +124,7 @@ function pydecref(o::PyObject)
end

function pyincref_(o::Union{PyPtr,PyObject})
ccall((@pysym :Py_IncRef), Cvoid, (PyPtr,), o)
@pyccall(:Py_IncRef, Cvoid, (PyPtr,), o)
Copy link
Member Author

@tkf tkf Sep 21, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

I guess Py_DecRef can call __del__ so it should probably be protected but I wonder if Py_IncRef can ever call any Python functions.

https://docs.python.org/3/c-api/typeobj.html#c.PyTypeObject.tp_dealloc

return o
end

Expand Down Expand Up @@ -150,13 +164,13 @@ function Base.copy!(dest::PyObject, src::PyObject)
end

pyisinstance(o::PyObject, t::PyObject) =
!ispynull(t) && ccall((@pysym :PyObject_IsInstance), Cint, (PyPtr,PyPtr), o, t.o) == 1
!ispynull(t) && @pyccall(:PyObject_IsInstance, Cint, (PyPtr,PyPtr), o, t.o) == 1

pyisinstance(o::PyObject, t::Union{Ptr{Cvoid},PyPtr}) =
t != C_NULL && ccall((@pysym :PyObject_IsInstance), Cint, (PyPtr,PyPtr), o, t) == 1
t != C_NULL && @pyccall(:PyObject_IsInstance, Cint, (PyPtr,PyPtr), o, t) == 1

pyquery(q::Ptr{Cvoid}, o::PyObject) =
ccall(q, Cint, (PyPtr,), o) == 1
@ccall(q, Cint, (PyPtr,), o) == 1

# conversion to pass PyObject as ccall arguments:
unsafe_convert(::Type{PyPtr}, po::PyObject) = po.o
Expand All @@ -171,7 +185,7 @@ PyObject(o::PyObject) = o
include("exception.jl")
include("gui.jl")

pytypeof(o::PyObject) = ispynull(o) ? throw(ArgumentError("NULL PyObjects have no Python type")) : PyObject(@pycheckn ccall(@pysym(:PyObject_Type), PyPtr, (PyPtr,), o))
pytypeof(o::PyObject) = ispynull(o) ? throw(ArgumentError("NULL PyObjects have no Python type")) : PyObject(@pycheckn @pyccall(:PyObject_Type, PyPtr, (PyPtr,), o))

#########################################################################

Expand Down Expand Up @@ -201,7 +215,7 @@ PyObject(o::PyPtr, keep::Any) = pyembed(PyObject(o), keep)
Return a string representation of `o` corresponding to `str(o)` in Python.
"""
pystr(o::PyObject) = convert(AbstractString,
PyObject(@pycheckn ccall((@pysym :PyObject_Str), PyPtr,
PyObject(@pycheckn @pyccall(:PyObject_Str, PyPtr,
(PyPtr,), o)))

"""
Expand All @@ -210,7 +224,7 @@ pystr(o::PyObject) = convert(AbstractString,
Return a string representation of `o` corresponding to `repr(o)` in Python.
"""
pyrepr(o::PyObject) = convert(AbstractString,
PyObject(@pycheckn ccall((@pysym :PyObject_Repr), PyPtr,
PyObject(@pycheckn @pyccall(:PyObject_Repr, PyPtr,
(PyPtr,), o)))

"""
Expand All @@ -224,10 +238,10 @@ function pystring(o::PyObject)
if ispynull(o)
return "NULL"
else
s = ccall((@pysym :PyObject_Repr), PyPtr, (PyPtr,), o)
s = @pyccall(:PyObject_Repr, PyPtr, (PyPtr,), o)
if (s == C_NULL)
pyerr_clear()
s = ccall((@pysym :PyObject_Str), PyPtr, (PyPtr,), o)
s = @pyccall(:PyObject_Str, PyPtr, (PyPtr,), o)
if (s == C_NULL)
pyerr_clear()
return string(o.o)
Expand Down Expand Up @@ -265,7 +279,7 @@ function hash(o::PyObject)
# since on 64-bit Windows the Python 2.x hash is only 32 bits
hashsalt(unsafe_pyjlwrap_to_objref(o.o))
else
h = ccall((@pysym :PyObject_Hash), Py_hash_t, (PyPtr,), o)
h = @pyccall(:PyObject_Hash, Py_hash_t, (PyPtr,), o)
if h == -1 # error
pyerr_clear()
return hashsalt(o.o)
Expand All @@ -283,7 +297,7 @@ function getindex(o::PyObject, s::AbstractString)
if ispynull(o)
throw(ArgumentError("ref of NULL PyObject"))
end
p = ccall((@pysym :PyObject_GetAttrString), PyPtr, (PyPtr, Cstring), o, s)
p = @pyccall(:PyObject_GetAttrString, PyPtr, (PyPtr, Cstring), o, s)
if p == C_NULL
pyerr_clear()
throw(KeyError(s))
Expand All @@ -297,7 +311,7 @@ function setindex!(o::PyObject, v, s::Union{Symbol,AbstractString})
if ispynull(o)
throw(ArgumentError("assign of NULL PyObject"))
end
if -1 == ccall((@pysym :PyObject_SetAttrString), Cint,
if -1 == @pyccall(:PyObject_SetAttrString, Cint,
(PyPtr, Cstring, PyPtr), o, s, PyObject(v))
pyerr_clear()
throw(KeyError(s))
Expand All @@ -309,7 +323,7 @@ function haskey(o::PyObject, s::Union{Symbol,AbstractString})
if ispynull(o)
throw(ArgumentError("haskey of NULL PyObject"))
end
return 1 == ccall((@pysym :PyObject_HasAttrString), Cint,
return 1 == @pyccall(:PyObject_HasAttrString, Cint,
(PyPtr, Cstring), o, s)
end

Expand Down Expand Up @@ -408,7 +422,7 @@ end
function _pyimport(name::AbstractString)
cookie = ActivatePyActCtx()
try
return PyObject(ccall((@pysym :PyImport_ImportModule), PyPtr, (Cstring,), name))
return PyObject(@pyccall(:PyImport_ImportModule, PyPtr, (Cstring,), name))
finally
DeactivatePyActCtx(cookie)
end
Expand Down Expand Up @@ -709,7 +723,7 @@ include("pyfncall.jl")
# for now we can define "get".

function get(o::PyObject, returntype::TypeTuple, k, default)
r = ccall((@pysym :PyObject_GetItem), PyPtr, (PyPtr,PyPtr), o,PyObject(k))
r = @pyccall(:PyObject_GetItem, PyPtr, (PyPtr,PyPtr), o,PyObject(k))
if r == C_NULL
pyerr_clear()
default
Expand All @@ -719,22 +733,22 @@ function get(o::PyObject, returntype::TypeTuple, k, default)
end

get(o::PyObject, returntype::TypeTuple, k) =
convert(returntype, PyObject(@pycheckn ccall((@pysym :PyObject_GetItem),
convert(returntype, PyObject(@pycheckn @pyccall(:PyObject_GetItem,
PyPtr, (PyPtr,PyPtr), o, PyObject(k))))

get(o::PyObject, k, default) = get(o, PyAny, k, default)
get(o::PyObject, k) = get(o, PyAny, k)

function delete!(o::PyObject, k)
e = ccall((@pysym :PyObject_DelItem), Cint, (PyPtr, PyPtr), o, PyObject(k))
e = @pyccall(:PyObject_DelItem, Cint, (PyPtr, PyPtr), o, PyObject(k))
if e == -1
pyerr_clear() # delete! ignores errors in Julia
end
return o
end

function set!(o::PyObject, k, v)
@pycheckz ccall((@pysym :PyObject_SetItem), Cint, (PyPtr, PyPtr, PyPtr),
@pycheckz @pyccall(:PyObject_SetItem, Cint, (PyPtr, PyPtr, PyPtr),
o, PyObject(k), PyObject(v))
v
end
Expand All @@ -751,24 +765,24 @@ function ind2py(i::Integer)
return i-1
end

_getindex(o::PyObject, i::Integer, T) = convert(T, PyObject(@pycheckn ccall((@pysym :PySequence_GetItem), PyPtr, (PyPtr, Int), o, ind2py(i))))
_getindex(o::PyObject, i::Integer, T) = convert(T, PyObject(@pycheckn @pyccall(:PySequence_GetItem, PyPtr, (PyPtr, Int), o, ind2py(i))))
getindex(o::PyObject, i::Integer) = _getindex(o, i, PyAny)
function setindex!(o::PyObject, v, i::Integer)
@pycheckz ccall((@pysym :PySequence_SetItem), Cint, (PyPtr, Int, PyPtr), o, ind2py(i), PyObject(v))
@pycheckz @pyccall(:PySequence_SetItem, Cint, (PyPtr, Int, PyPtr), o, ind2py(i), PyObject(v))
v
end
getindex(o::PyObject, i1::Integer, i2::Integer) = get(o, (ind2py(i1),ind2py(i2)))
setindex!(o::PyObject, v, i1::Integer, i2::Integer) = set!(o, (ind2py(i1),ind2py(i2)), v)
getindex(o::PyObject, I::Integer...) = get(o, map(ind2py, I))
setindex!(o::PyObject, v, I::Integer...) = set!(o, map(ind2py, I), v)
length(o::PyObject) = @pycheckz ccall((@pysym :PySequence_Size), Int, (PyPtr,), o)
length(o::PyObject) = @pycheckz @pyccall(:PySequence_Size, Int, (PyPtr,), o)
size(o::PyObject) = (length(o),)
firstindex(o::PyObject) = 1
lastindex(o::PyObject) = length(o)

function splice!(a::PyObject, i::Integer)
v = a[i]
@pycheckz ccall((@pysym :PySequence_DelItem), Cint, (PyPtr, Int), a, i-1)
@pycheckz @pyccall(:PySequence_DelItem, Cint, (PyPtr, Int), a, i-1)
v
end

Expand All @@ -777,20 +791,20 @@ popfirst!(a::PyObject) = splice!(a, 1)

function empty!(a::PyObject)
for i in length(a):-1:1
@pycheckz ccall((@pysym :PySequence_DelItem), Cint, (PyPtr, Int), a, i-1)
@pycheckz @pyccall(:PySequence_DelItem, Cint, (PyPtr, Int), a, i-1)
end
a
end

# The following operations only work for the list type and subtypes thereof:
function push!(a::PyObject, item)
@pycheckz ccall((@pysym :PyList_Append), Cint, (PyPtr, PyPtr),
@pycheckz @pyccall(:PyList_Append, Cint, (PyPtr, PyPtr),
a, PyObject(item))
a
end

function insert!(a::PyObject, i::Integer, item)
@pycheckz ccall((@pysym :PyList_Insert), Cint, (PyPtr, Int, PyPtr),
@pycheckz @pyccall(:PyList_Insert, Cint, (PyPtr, Int, PyPtr),
a, ind2py(i), PyObject(item))
a
end
Expand All @@ -816,7 +830,7 @@ function append!(a::PyObject, items)
end

append!(a::PyObject, items::PyObject) =
PyObject(@pycheckn ccall((@pysym :PySequence_InPlaceConcat),
PyObject(@pycheckn @pyccall(:PySequence_InPlaceConcat,
PyPtr, (PyPtr, PyPtr), a, items))

#########################################################################
Expand Down
4 changes: 3 additions & 1 deletion src/callback.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,9 @@ function _pyjlwrap_call(f, args_::PyPtr, kw_::PyPtr)
end

pyjlwrap_call(self_::PyPtr, args_::PyPtr, kw_::PyPtr) =
_pyjlwrap_call(unsafe_pyjlwrap_to_objref(self_), args_, kw_)
reenable_sigint() do
_pyjlwrap_call(unsafe_pyjlwrap_to_objref(self_), args_, kw_)
end

################################################################
# allow the user to convert a Julia function into a Python
Expand Down
Loading