Skip to content

Weakref callback #488

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

Closed

Conversation

BenLewis-Seequent
Copy link

I have made changes to allow weak references to have callbacks, but I have ran into a problem about how to run these callbacks during drop as we have no reference in the virual machine within the drop method.

I see three possible solutions to this:

  1. Add PyObjectRef::drop(vm: &mut VirtualMachine, obj: PyObjectRef) which would have be called to drop each reference. There is no way to make the compiler enforce this, so this could lead to plenty of bugs caused by not being dropped correctly.
  2. Make it possible to invoke Python code without having to be passed a mutable reference to the virtual machine. This would likely involve making fine locks for the parts of the virtual machine that needs to be locked. This is effectively 'Remove GIL'.
  3. Don't immediately invoke the callbacks/__del__ and do it later:
    • Either when control returns to the core interpreter
    • Or don't use reference counting at all, instead use a mark and sweep GC which can be given the mutable reference to the virtual machine when we want to collect garbage.

@windelbouwman
Copy link
Contributor

I faced the exact same issue when implementing dictionaries. There was no way to refer to the virtual machine from within the Eq trait in that case. You need to be able to call python code, for this you require a virtualmachine. Next to that, there is another issue, the python code can fail with an exception. How do you handle this exception?

@BenLewis-Seequent
Copy link
Author

BenLewis-Seequent commented Feb 17, 2019

The documentation for weakref states:
"Exceptions raised by the callback will be noted on the standard error output, but cannot be propagated; they are handled in exactly the same way as exceptions raised from an object’s __del__() method.". So exceptions isn't a problem as they can't be propagated out of the drop method.

@BenLewis-Seequent BenLewis-Seequent marked this pull request as ready for review February 19, 2019 08:41
@BenLewis-Seequent
Copy link
Author

I have gone with approach 3 of, putting objects that are about to be deleted into a queue and processing them after every frame and del statement. This is the simplest approach to get working.

@codecov-io
Copy link

codecov-io commented Feb 19, 2019

Codecov Report

Merging #488 into master will decrease coverage by 4.16%.
The diff coverage is 58.79%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #488      +/-   ##
==========================================
- Coverage   57.38%   53.21%   -4.17%     
==========================================
  Files          67       68       +1     
  Lines       12300    13167     +867     
  Branches     2906     3198     +292     
==========================================
- Hits         7058     7007      -51     
- Misses       3457     4349     +892     
- Partials     1785     1811      +26
Impacted Files Coverage Δ
vm/src/obj/objstr.rs 59.29% <ø> (ø) ⬆️
vm/src/sysmodule.rs 42.85% <0%> (-18.85%) ⬇️
vm/src/obj/objtype.rs 60.69% <0%> (ø) ⬆️
vm/src/builtins.rs 35.69% <100%> (-20.86%) ⬇️
vm/src/stdlib/weakref.rs 14.7% <100%> (-61.16%) ⬇️
vm/src/frame.rs 46.81% <33.33%> (-5.32%) ⬇️
vm/src/pyobject.rs 60.39% <53.78%> (-24.39%) ⬇️
vm/src/obj/objweakref.rs 73.58% <73.58%> (ø)
... and 8 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update fcea845...350bc30. Read the comment docs.

@@ -697,6 +801,7 @@ impl PyContext {
pub struct PyObject {
pub payload: PyObjectPayload,
pub typ: Option<PyObjectRef>,
weak_refs: Vec<PyObjectWeakRef>,
Copy link
Contributor

@adrian17 adrian17 Feb 19, 2019

Choose a reason for hiding this comment

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

that's quite a heavy approach. AFAIK In CPython, weakref support is optional (stored in payload, signaled at type object level and explicitly checked only in destructors of classes that opted into it), and disabled for most builtin types to save on memory and time.
Guessing that's related to #371 too though.

Copy link
Author

Choose a reason for hiding this comment

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

I have moved the attribute into the 'Instance' payload as, as you said not every object needs to be weak referenceable. We need to keep in mind for #371 as to how to keep track of these weak references.

It's only costly in terms of space as checking for the presence of weak references would just be checking the length of the vector, which is directly in the PyObject.

@windelbouwman
Copy link
Contributor

The documentation for weakref states:
"Exceptions raised by the callback will be noted on the standard error output, but cannot be propagated; they are handled in exactly the same way as exceptions raised from an object’s del() method.". So exceptions isn't a problem as they can't be propagated out of the drop method.

I think it is fair to deviate here from cpython. This is pretty unacceptable to silently take up an error. At least the interpreter should crash in some clean way. Maybe by propagating the exception to top loop?

@BenLewis-Seequent
Copy link
Author

For exceptions like SystemExit and KeyboardInterrupt they need to be propagated, so that they can tell the vm to shutdown.

A consequence of propagating an exception from __del__ is that any exception can come from any function, making it difficult to reason about what exceptions a method could raise. Another problem in CPython is that the tracebacks holds references to all frames therefore all local variables including the deleted reference. Also it makes observing when garbage collection implementation calls __del__ easier, making it possibly harder to change it's behavior in the future.

Shutting the whole vm down due to an exception in __del__ is a significant deviation as it will mean Python programs will be less resilient to errors. Normally catching exceptions within a main loop protects the program from crashing, and can recover from normal exceptions.

A correction is that sys.excepthook is actually what is called to handle the uncaught exception from __del__, allowing the ability to override this handling. Note it's not passed a traceback due to the problem I mentioned earlier about holding references to the deleted object.

@BenLewis-Seequent
Copy link
Author

This is now a bit outdated with the recent changes to the payload, so I will close this, and reimplement it in a series of a couple of PRs, the first of which is #636.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants