-
Notifications
You must be signed in to change notification settings - Fork 747
Add PyHandle #1087
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
Add PyHandle #1087
Conversation
As far as I can see, However, implicit conversions destroy the whole point of There are two reasons I did not add more explicit type to
Overall the way I see correct refcount handling could work is:
|
|
||
namespace Python.Runtime | ||
{ | ||
struct PyHandle : IDisposable |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Having regular structs IDisposable
is a bad idea. The following two cases would cause the pointer to be copied and potentially lead to double free:
var x = GetNewPyHandleToSomething();
var y = x;
x.Dispose();
y.Dispose();
var x = GetNewPyHandleToSomething();
void DoSomethingWithHandle(PyHandle handle) {
...
handle.Dispose();
}
DoSomethingWithHandle(x);
x.Dispose();
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This merely for easy using statement like using(var list = PyList_New())
, people should know what they're doing when they using the C API, if they're not, everything on language sight are just smoke and mirrors. Just like you can always decref a wrong object.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@amos402 but they only have to use raw C API if you expose it. In that sense *Reference
types are fool-proof, unless you use DagerousGetAddress
.
C-style API is a error-prone mental burden, which is totally unnecessary in this case.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not agree with it, since you can't control the user how write the code, these fool-proof are helpless.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@amos402 users of Python.NET do not need to work with pointers at all.
In PRs for Python.NET I can safely skip attempts to verify correct refcount balancing if the code does not use .DangerousGetAddress()
, which will make them easier to read.
Also, I started with little conversions to |
That's what I wanted, we need a equivalent type for I don't like current usages of the var obj = PyList_New();
// do something
// return and didn't call Decref or Dispose As a result, the same as you use |
@amos402 actually, I recently upstreamed a change to FxCop, that will make your example fail at compile time because you did not call But even without it, the new types already prevent accidental copying without corresponding increfs. |
That means I even can just return the returned value. I have to incref it first, and store its pointer from |
@amos402 no, it does not. Static escape analyzer is smart enough to extend scope when the value is returned. You can actually see how this works right now with |
I don't like that |
Can you show a scenario in Python.NET code? |
NewReference pylist = Runtime.PyList_New(10);
Runtime.PyList_Append(pylist, pyobj); // X
using (NewReference pylist = Runtime.PyList_New(10)) // X
{
}
List<NewReference> list1; // X
list1.Add(pylist); // X
List<BorrowoedReference> list2; // X
list2.Add(PySys_GetObject("modues") // X |
@amos402 I meant a full method, that does something useful. BTW, the first X should not fail. I think The second X fails because we use C# 7.3. It would work in C# 8.0, which we could switch to after 2.4 is released. |
I thought the usages above are just some general usages, but it can't do it. If you need the concrete usages, like these: Generator call: X |
@amos402 the first example will have only one The second scenario is indeed dangerous, and I feel like it is good to have it indicated as such. You are serializing pointers, that would live across domain boundary and Python interpreter instantiations. |
Why you have to specific they're New or Borrowed precisely when they as parameters, wouldn't it make confuse to others? Is not easy to treat the parameters as PyHandle and return
Please don't rely on the language version much. |
@amos402 most parameters are |
That means it may confuse the concept of new reference and borrowed reference, steals the reference means it just would not add reference count, now you say the parameter is a new reference object, things become jumble. |
@amos402 I think it is pretty clear. If a parameter is |
BTW, opened a PR for allowing to borrow from a |
var xxx = PySysGetObject("xxx"); // borrowed
PyTuple_SetItem(t, xxx); // no no no, you're passing a BorrowedReference, you need to make it to NewReference
var xxx = PyObject_GetString(obj, "list"); // new
PyList_Append(xxx, obj); // no no no, you're passing a NewReference, you need to make it to BorrowedReference. Oh, seems it can be upgrade automatically, fine.
PyList_SetItem(xxx, xxx); // no no no, you're passing a NewReference, you need to make it to BorrowedReference. Can you see that, it can be upgrade automatically, OK... Wait, `xxx` is what, it's a new or borrowed, or something else..... Now then, let's check the manual and ignore what the signature said..... You are increasing the use-cost. |
For reference stealing we can have another non-copyable public StolenReference Steal() => ... Methods to both PyList_SetItem(listReference, index, itemReference.Steal()); Alternatively, we could find a PInvoke trick, that would allows us to use |
Oh no, another type, that's what am saying, things become jumble. Do we really need this? I though just enough by using PyHandle, no tricks. |
var xxx = PySysGetObject("xxx"); // borrowed
PyTuple_SetItem(t, xxx); // no no no, you're passing a BorrowedReference, you need to make it to NewReference And that's exactly the point! You have to construct a suitable new reference for var xxx = PyObject_GetString(obj, "list"); // new
PyList_Append(xxx, obj); // no no no, you're passing a NewReference, you need to make it to BorrowedReference. Oh, seems it can be upgrade automatically, fine. As you say, can be fixed by #1080. PyList_SetItem(xxx, xxx); // no no no, you're passing a NewReference, you need to make it to BorrowedReference. Can you see that, it can be upgrade automatically, OK... Wait, `xxx` is what, it's a new or borrowed, or something else..... Now then, let's check the manual and ignore what the signature said..... That scenario I am still thinking about. With the PyList_SetItem(xxx, xxx.Steal());
// a person reading code can see reference is stolen without referring to Python docs
PyList_SetItem(xxx, xxx); // <- won't compile! excellent: you forgot to IncRef xxx! |
I mean, I wouldn't say that's a NewReference, why it named New? It's not NEW. All I just want to use the Python API just as write the C/C++ extension. People who use these API should always care about the reference. Since there's no smart pointer here, the operates handle the reference count does necessity, you can control them automatically either. But these types make code become verbose and ambiguous. |
@amos402 , again, this part (where parameter is to be stolen) is not settled yet. PyTuple_SetItem(t, xxx.NewReference()); or like this: PyTuple_SetItem(t, xxx.Steal()); But I am afraid we'd have to resort to something like |
Return value is just fine, but introduce them into parameters, I refuse. |
@amos402 any specific reasons? I have one for the distinct types: when I read a pull request, I'd like to avoid looking up every single Python function call I encounter in docs to ensure the pull request author counted |
Yes, the type checking may be a little help for that. But it should not stuck the general usage. So I tend to use implicit operator to make them become PyHandle, user should know that they're just using the raw pointer when they use it, because you have to declare it as |
I'm sorry, but no. This mindset got us exactly all of those reference count issues we (and in particular you) have been working on for years now to resolve. There is no shame in getting the compiler to help out on this. Naming can be discussed, but with this the code becomes /less/ ambiguous because we don't (ab)use |
I have to declare that I didn't critical the Rererence types.
p.s. If I used any wrong English grammar or words before, please point out them if it won't bother you too much😝. |
We are not taking this change. |
What does this implement/fix? Explain your changes.
PyHandle
to replaceIntPtr
of C API.Rutime.XDecref
andRuntime.XIncref
.BorrowedReference
andNewReference
NewReference
andBorrowedReference
seem be good for helping user comprehend the type of return value quickly. But it should not interfere the general usage.When I tried to merge the code, I found that the
DangerousGetAddress
just keep annoying me. In many cases we have to use the rawIntPtr
(e.g. put the Python object to a clr collection). Keep typing the long method nameDangerousGetAddress
just verbose since I must know that C API does return borrowed reference or new reference before using the C API.Also I can't the
NewReference
orBorrowedReference
to the collection, because they're non-copyable. This may good cause no matter which put to the collection wouldn't make sense(e.g.List<
NewReference>`, I still have to iterate it and Decref the objects when I clear the List).So, here by I introduce the
PyHandle
. Be more readable compare with the rawIntPtr
, it contains some basic functions for the Python object for easy usage, can be implicit convert fromNewReference
orBorrowedReference
. And it doesn't conflict with current code.Use the
PyHandle
instead ofBorrowedReference
as the parameter type should be more rantional.For example, such as
int PyList_Append(BorrowedReference pointer, IntPtr value);
, why thepointer
should be aBorrowedReference
? What about thevalue
what's type it should be? EitherNew
norBorrowed
both doesn't match.I may just created a PyList from
PyList_New
and it will return aNewReference
, but I can't even use the return value through to the other functions, seems that doesn't match the general usage cases.Does this close any currently open issues?
#1068
#1043
Any other comments?
...
Checklist
Check all those that are applicable and complete.
AUTHORS
CHANGELOG