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.

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

Clean up globals defined in Python source, make sure their __del__ is called #988

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
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
33 changes: 22 additions & 11 deletions Src/IronPython/Runtime/PythonContext.cs
Original file line number Diff line number Diff line change
Expand Up @@ -271,7 +271,7 @@ public PythonContext(ScriptDomainManager/*!*/ manager, IDictionary<string, objec
try {
HookAssemblyResolve();
} catch (System.Security.SecurityException) {
// We may not have SecurityPermissionFlag.ControlAppDomain.
// We may not have SecurityPermissionFlag.ControlAppDomain.
// If so, we will not look up sys.path for module loads
}
}
Expand Down Expand Up @@ -1238,7 +1238,7 @@ private void UnhookAssemblyResolve() {
try {
AppDomain.CurrentDomain.AssemblyResolve -= _resolveHolder.AssemblyResolveEvent;
} catch (System.Security.SecurityException) {
// We may not have SecurityPermissionFlag.ControlAppDomain.
// We may not have SecurityPermissionFlag.ControlAppDomain.
// If so, we will not look up sys.path for module loads
}
}
Expand Down Expand Up @@ -1295,6 +1295,17 @@ public override void Shutdown() {
}
#endif
}

// clean up globals from modules
foreach (var module in SystemStateModules.Values) {
if (module is PythonModule pyModule) {
pyModule.Cleanup(SharedContext);
}
}

// allow finalizers to run before shutdown
GC.Collect();
GC.WaitForPendingFinalizers();
}

// TODO: ExceptionFormatter service
Expand Down Expand Up @@ -1569,7 +1580,7 @@ public override TService GetService<TService>(params object[] args) {

/// <summary>
/// Returns (and creates if necessary) the PythonService that is associated with this PythonContext.
///
///
/// The PythonService is used for providing remoted convenience helpers for the DLR hosting APIs.
/// </summary>
internal Hosting.PythonService GetPythonService(Microsoft.Scripting.Hosting.ScriptEngine engine) {
Expand Down Expand Up @@ -1678,7 +1689,7 @@ private Dictionary<string, Type> CreateBuiltinTable() {

if (Environment.OSVersion.Platform == PlatformID.Unix) {
// we make our nt package show up as a posix package
// on unix platforms. Because we build on top of the
// on unix platforms. Because we build on top of the
// CLI for all file operations we should be good from
// there, but modules that check for the presence of
// names (e.g. os) will do the right thing.
Expand Down Expand Up @@ -2529,8 +2540,8 @@ private CallSite<Func<CallSite, object, T>> MakeConvertSite<T>(ConversionResultK

/// <summary>
/// Invokes the specified operation on the provided arguments and returns the new resulting value.
///
/// operation is usually a value from StandardOperators (standard CLR/DLR operator) or
///
/// operation is usually a value from StandardOperators (standard CLR/DLR operator) or
/// OperatorStrings (a Python specific operator)
/// </summary>
internal object Operation(PythonOperationKind operation, object self, object other) {
Expand Down Expand Up @@ -2987,13 +2998,13 @@ internal CultureInfo NumericCulture {
/// <summary>
/// Sets the current command dispatcher for the Python command line. The previous dispatcher
/// is returned. Null can be passed to remove the current command dispatcher.
///
///
/// The command dispatcher will be called with a delegate to be executed. The command dispatcher
/// should invoke the target delegate in the desired context.
///
///
/// A common use for this is to enable running all REPL commands on the UI thread while the REPL
/// continues to run on a non-UI thread.
///
///
/// The ipy.exe REPL will call into PythonContext.DispatchCommand to dispatch each execution to
/// the correct thread. Other REPLs can do the same to support this functionality as well.
/// </summary>
Expand Down Expand Up @@ -3094,8 +3105,8 @@ public int Compare(object x, object y) {
/// <summary>
/// Gets a function which can be used for comparing two values using the normal
/// Python semantics.
///
/// If type is null then a generic comparison function is returned. If type is
///
/// If type is null then a generic comparison function is returned. If type is
/// not null a comparison function is returned that's used for just that type.
/// </summary>
internal IComparer GetComparer(Type type) {
Expand Down
34 changes: 30 additions & 4 deletions Src/IronPython/Runtime/PythonModule.cs
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@
using System.Collections.Generic;
using System.Diagnostics;
using System.Dynamic;
using System.Linq;
using System.Runtime.CompilerServices;
using System.Threading;

Expand All @@ -30,6 +31,7 @@ namespace IronPython.Runtime {
public class PythonModule : IDynamicMetaObjectProvider, IPythonMembersList {
private readonly PythonDictionary _dict;
private Scope _scope;
private bool _cleanedUp = false;

public PythonModule() {
_dict = new PythonDictionary();
Expand All @@ -49,7 +51,7 @@ internal PythonModule(PythonContext context, Scope scope) {

/// <summary>
/// Creates a new PythonModule with the specified dictionary.
///
///
/// Used for creating modules for builtin modules which don't have any code associated with them.
/// </summary>
internal PythonModule(PythonDictionary dict) {
Expand Down Expand Up @@ -253,7 +255,7 @@ public DynamicMetaObject GetMember(PythonGetMemberBinder member, DynamicMetaObje

private DynamicMetaObject GetMemberWorker(DynamicMetaObjectBinder binder, DynamicMetaObject codeContext) {
string name = GetGetMemberName(binder);
var tmp = Expression.Variable(typeof(object), "res");
var tmp = Expression.Variable(typeof(object), "res");

return new DynamicMetaObject(
Expression.Block(
Expand Down Expand Up @@ -355,7 +357,7 @@ IList<string> IMembersList.GetMemberNames() {
}

#endregion

internal class DebugProxy {
private readonly PythonModule _module;

Expand All @@ -375,6 +377,30 @@ public List<ObjectDebugView> Members {
}
}


/// <summary>
/// Cleanup globals so that they could be garbage collected.
/// Note that it cleans python sourced modules only,
/// because C# modules are more fundamental and their globals may be required when other python object's finalizer is executing.
/// </summary>
public void Cleanup(CodeContext context) {
if (_cleanedUp) {
return;
}

_cleanedUp = true;

if (!_dict.ContainsKey("__file__")) {
return; // not from Python source, skip clean up
}

foreach (var key in _dict.Keys.ToList()) {
var obj = _dict[key];
if (obj is PythonModule module) {
module.Cleanup(context);
} else if (key is string name) {
__delattr__(context, name);
}
}
}
}
}
41 changes: 41 additions & 0 deletions Tests/test_regressions.py
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ def test_cp1234(): ...
import os
import sys
import unittest
from subprocess import check_output

from iptest import IronPythonTestCase, is_cli, is_mono, is_netcoreapp, is_posix, run_test, skipUnlessIronPython, stdout_trapper

Expand Down Expand Up @@ -1467,4 +1468,44 @@ def get_class_class(cls):
self.assertEqual(o.get_self_class(), o.get_class())
self.assertEqual(o.get_class(), o.get_class_class())

@unittest.skipIf(is_mono, 'https://github.com/IronLanguages/ironpython3/issues/937')
def test_ipy3_gh985(self):
"""https://github.com/IronLanguages/ironpython3/issues/985"""
code = """
import sys


class Foo:
def __init__(self, name):
self.name = name

def __del__(self):
sys.stdout.write(self.name + '\\n')


def func():
foo = Foo('Foo 1')
bar = Foo('Foo 2')
del bar


func()


foo = Foo('Foo 3')
foo = Foo('Foo 4')
"""

test_script_name = os.path.join(self.test_dir, 'ipy3_gh985.py')
f = open(test_script_name, 'w')
try:
f.write(code)
f.close()

output = check_output([sys.executable, test_script_name]).decode(sys.stdout.encoding).strip()
self.assertEqual(set(output.split(os.linesep)), {'Foo 1', 'Foo 2', 'Foo 3', 'Foo 4'})
finally:
os.unlink(test_script_name)


run_test(__name__)