@filmor actually Decref only it prints Decref(NULL) if IntPtr.Zero. We have a lot of cases where IntPtr.Zero is checked before Decref. But I agree that use case here is missing.

Also there are 2 places where Runtime.Decref(self.repr) is called in the same file.

The check was the problem, actually: it calls
DebugUtil.Print("Decref(NULL)");
which tries to print to the Console, which sometimes throws an

Exception thrown: 'System.IO.IOException' in mscorlib.dll
Additional information: The handle is invalid.

then (for some reason I don't fully understand)

I found the reason for the IOException: it is due to this:

Console class members that work normally when the underlying stream is directed to a console might throw an exception if the stream is redirected, for example, to a file. Program your application to catch System.IO.IOException exceptions if you redirect a standard stream. You can also use the IsOutputRedirected, IsInputRedirected, and IsErrorRedirected properties to determine whether a standard stream is redirected before performing an operation that throws an System.IO.IOException exception.

from https://msdn.microsoft.com/en-us/library/system.console.aspx?f=255&MSPPError=-2147217396

Even without those problems the spurious "Decref("NULL")" error messages are pretty annoying.

Any objections to this change?

I agree with this change but why not change in both places?

On Thu, Oct 20, 2016 at 8:53 AM, ArvidJB notifications@github.com wrote:

I found the reason for the IOException: it is due to this:

Console class members that work normally when the underlying stream is
directed to a console might throw an exception if the stream is redirected,
for example, to a file. Program your application to catch
System.IO.IOException exceptions if you redirect a standard stream. You can
also use the IsOutputRedirected, IsInputRedirected, and IsErrorRedirected
properties to determine whether a standard stream is redirected before
performing an operation that throws an System.IO.IOException exception.

from https://msdn.microsoft.com/en-us/library/system.console.
aspx?f=255&MSPPError=-2147217396

Even without those problems the spurious "Decref("NULL")" error messages
are pretty annoying.

Any objections to this change?

—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
#275 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AHgZ5cU3bBt2jEc_2hxZTctxidgHIrIoks5q13JsgaJpZM4KbPz8
.

Well, isn't the right thing to do then fixing/removing the debug log and documenting that Incref and Decref are safe to call on null?

EDIT: Just to clarify, even if the code does not go into the home-made decref (https://github.com/pythonnet/pythonnet/blob/master/src/runtime/runtime.cs#L567), it calls Py_DecRef which according to the Python docs is an exported version of Py_XDECREF which does the extra NULL check. There's no point in doing that twice nor log about that.

Is it really safe to call Decref on IntPtr.Zero? Why does pythonnet
have these ~9 checks all over the code?

Find all "if.*\!\=.*IntPtr.Zero.*\n.*(\n.*){0,7}Runtime.Decref", Regular
expressions, Find Results 1, Entire Solution, ""
  C:\Python\pythonnet\pythonnet\src\runtime\classbase.cs(238):
 if (dict != IntPtr.Zero)
  C:\Python\pythonnet\pythonnet\src\runtime\classderived.cs(850):
             if (dict != IntPtr.Zero)
  C:\Python\pythonnet\pythonnet\src\runtime\converter.cs(297):
           if (p != IntPtr.Zero)
  C:\Python\pythonnet\pythonnet\src\runtime\converter.cs(307):
       if ((c != IntPtr.Zero) && (c != value))
  C:\Python\pythonnet\pythonnet\src\runtime\exceptions.cs(196):
       if (op != IntPtr.Zero)
  C:\Python\pythonnet\pythonnet\src\runtime\managedtype.cs(51):
       if ((e != IntPtr.Zero) && (e != ob))
  C:\Python\pythonnet\pythonnet\src\runtime\metatype.cs(177):            if
(init != IntPtr.Zero)
  C:\Python\pythonnet\pythonnet\src\runtime\methodbinder.cs(336):
                     if (pyoptype != IntPtr.Zero)
  C:\Python\pythonnet\pythonnet\src\runtime\methodbinding.cs(164):
                   if (baseMethod != IntPtr.Zero)
  C:\Python\pythonnet\pythonnet\src\runtime\pythonexception.cs(155):
             if (_pyTB != IntPtr.Zero)
  Matching lines: 10    Matching files: 9    Total files searched: 99

On Thu, Oct 20, 2016 at 9:16 AM, Benedikt Reinartz <notifications@github.com

wrote:

Well, isn't the right thing to do then fixing/removing the debug log and
documenting that Incref and Decref are safe to call on null?

—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
#275 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AHgZ5R5HLnFLNwwXb6ODQlW2uWWEZ5NLks5q13fZgaJpZM4KbPz8
.

@denfromufa i made the other change as well. I missed it since it looks like we were not hitting that problem in ConstructorBinding.tp_dealloc in our codebase.
@filmor I think this is actually a useful message since it basically tells you that somewhere you did some inconsistent reference counting.

@denfromufa I don't know why those checks are in, but just look at the function, both implementations (with Py_DEBUG set and without) check explicitly for the pointer being 0.
@ArvidJB Not really. If our reference counting is actually off in an unsafe way we would never notice, as pointers are not set to 0 when they are freed by Decref.

I vote we remove the message instead and document the behaviour.

I agree that DebugUtil.Print() is a useful method and it should probably be
a SEPARATE PR to improve it for cases when console output is redirected.

So I agree with this pull request now, but generally we have a lot of other
places where ref count is not checked for null before decrementing.

On Thu, Oct 20, 2016 at 10:57 AM, ArvidJB notifications@github.com wrote:

@denfromufa https://github.com/denfromufa i made the other change as
well. I missed it since it looks like we were not hitting that problem in
ConstructorBinding.tp_dealloc in our codebase.
@filmor https://github.com/filmor I think this is actually a useful
message since it basically tells you that somewhere you did some
inconsistent reference counting.

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#275 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AHgZ5ebgxZPkqND7fIGbsCMXiGfE3v9Tks5q149lgaJpZM4KbPz8
.

@denfromufa sounds good to me.
I will try to see how easy it is to fix the general problem of writing to the Console. Maybe it's as easy as writing to stdout directly?

@filmor do you mean that this line below is the check for the pointer being zero?

Where is equivalent check for Py_DEBUG version? I did not find this!

@ArvidJB somehow my previous message that I sent using gmail was pushed/updated only 9 hours ago on github. But in my gmail I sent it 5 days ago, before @filmor 's last message. This is just to clarify the sequence of messages we had 5 days ago.

@tonyroberts @vmuriart do you guys have any better suggestions for this pull request?

In my opinion, we need to address 2 questions:

1. Is it safe to Decref without checking for IntPtr.Zero? Hopefully someone can write a test for this, if this is safe. And then all ~9 manual checks that I referenced above have to be removed from pythonnet codebase.
2. Do we need a debug message when trying to decref IntPtr.Zero? If yes, how to handle it safely for redirected outputs?

Just saw filmor's earlier comment that the exported Py_DecRef function is actually Py_XDECREF.

What I would do is rename Decref to Py_XDECREF and keep the null check and remove the logging - then it's clear to everyone that it can be called safely with a null pointer.

@tonyroberts thanks for pointing to @filmor 's edit above. Indeed, I did not notice it either. Especially, since I was looking at my email. So new messages about edits are generally better + optional in-place edits.

@ArvidJB now I completely agree with the latest comments from @filmor and @tonyroberts. Let me know if anything is not clear? Are you planning to continue with this pull request?

@denfromufa That makes sense, I will try to make that change. Should I open a new pull request for that or is it okay to continue on this one?

We can continue on this pull request and from your branch. Once ready, we
can just squash your commits into "logical unit(s)".

On Wed, Oct 26, 2016 at 11:12 AM, ArvidJB notifications@github.com wrote:

@denfromufa https://github.com/denfromufa That makes sense, I will try
to make that change. Should I open a new pull request for that or is it
okay to continue on this one?

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#275 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AHgZ5Uv7pg30Y99r8zc-J_tGtdhfjA__ks5q33wBgaJpZM4KbPz8
.

I renamed Incref/Decref to XIncref/XDecref instead of Py_XINCREF/Py_XDECREF since that felt like a more C# appropriate name. I removed the additional NULL check I had added and also removed the code that prints on NULL in Decref.

looks like @tonyroberts approved your commit @ArvidJB

@filmor do you agree with these changes?

I agree, but please rename and squash the PR before merging.

@filmor now we can squash or rebase and merge pull requests with one button click (select a drop-down on merge pull request). More info in this article:

https://github.com/blog/2141-squash-your-commits