I'm wondering if it would be better/simpler to define PyObjectRc as something like:

struct PyObjectRc {
    inner: Arc<...>
}

impl Drop for PyObjectRc {
    fn drop(&mut self) {
        if self.inner.strong_count() == 1 {
            // call __del__
        }
        // drop Arc
    }
}

The only problem i see with this is that there is a race condition if two references are dropped at the same time, but I think a lock(possibly a global lock) just needs to be added to ensure the drop of the Arc and checking of strong count are atomic. But we only need to do that if the object has a del method in the first place.

Yeah, that's what I was thinking -- we could put a ManuallyDrop around the inner arc, and either ManuallyDrop::drop() or ManuallyDrop::take() the field.

@Skinny121 thanks, idk why I didn't think there must be public interface for that.
@coolreader18 I have no idea how manually drop help it. could you tell me more about the idea? as i know, ManuallyDrop is good tool to reorder the fields drop but container and fields are always drop as reverse order of construction. So container first and fields later - it seems nowhere to use ManuallyDrop between wrapper and Arc. Probably you are thinking about somewhere else?

The key would be for ManuallyDrop::take(); I don't think it's really feasible to use the thread local VM and call __del__ directly inside drop(), since a PyObject could be stored somewhere else and dropped while the VM isn't active; I think we should try to do something like have a queue of objects that need to be destructed, and check it periodically like we do check_signals

And even if we do go with the thread local VM, it's tricky to call __del__ while you still have to clone the inner field. If you take() it and pass it to __del__ then the reference count of self while inside the destructor will be 1.

Edit: Nevermind, that would probably lead to issues with infinitely recurring into a destructor

Ok, I got what you meant about ManuallyDrop.

About __del__ timing, it is easy to avoid by one of below:

• Easy way: increase count, run __del__, decrease count
• Harder way: Create arc with count 2 and drop when it goes to count 2. decrease 1 more and drop arc

The thread_local problem:
I agree current way is not ideal. I didn't look up exact point to safely set the shared value. I think we at least need a thread local variable regardless it is a vm, an executing frame or a vec (for sweeping). I set it on frame because It was the safetest place for every choice, but it can be improved in future if the value is vm. things will be easier when a thread is always bound by a vm.

I don't know well about vm, gc and their performance that much.
But I am thinking about PyPy's pros and cons by its design.

I believe there is an opportunity of better performance for collecting and release.

I am thinking of making RustPython as a drop-in-replacement of CPython.
For this purpose, it requires a sort of 'hard compatibility' of CPython.
I don't think it is specific future of RustPython, but at lease we can consider a mode like that. Hard-compatibilty will give potential opportunity between CPython and PyPy.

One of the common problem is resource releasing timing. Some (wrong) python code is depending of CPython's drop timing, which only can simulated by calling __del__ at exact timing to go to rc=0. So, I want to have this one as a possible memory management model, unless we can prove RustPython never can implements c/c++ extension interface.

Ah, it's an issue with stuff being dropped while in vm.initialize(), I'll fix that

@Skinny121 what do you think?

Looks pretty good. PyObjectRc's looks like they can still escape crate::vm::thread::enter_vm though, which means that this doesn't guarantee that a vm is always available in the drop. The way to ensure this is to add a lifetime parameter to PyObjectRc, though I'm unsure how feasible that would actually be though.

I added another commit to rework vm initialization a bit; not sure if it's totally on-topic but I think it makes more sense with how the Interpreter struct works now, and I think it's good to have the InitParameter be more explicit, since either way having an expectation for how it should work by default isn't great. If you expect it by default not to have access to external imports but it does, that could be a security risk, and if you expect it by default to have access to external imports but it doesn't, that might be confusing why something like import os fails.

Should we merge this?

If the design is good enough, let's go and fix remained problems later. this pr is growing bigger. bigger patch will cause more frequent conflicts for other PRs

I think the comments here is related to #1940