Domain reload test cases fixes #1287

BadSingleton · 2020-11-19T16:57:33Z

What does this implement/fix? Explain your changes.

This PR adds the fixes for the xfail tests in #1275 .

Serialization of System.Type, MemberInfo and MethodBase is now string based. At de-serialization, use reflection to attempt to recreate the object, which now fails safely instead of throwing a SerializationException during the de-serialization of the whole data stream. Appropriate Exceptions will now be raised when the Maybe*'s Value property is accessed.

Does this close any currently open issues?

This addresses #957 and #1250 and offers an answer to #1268 .

Any other comments?

TODO: add changelog entries once the fixes are approved. Also, add docs on best practices with domain reloads.

Checklist

Check all those that are applicable and complete.

Make sure to include one or more tests for your change
If an enhancement PR, please create docs and at best an example
Add yourself to AUTHORS
Updated the CHANGELOG

The tests are marked as expected failures for now.

…t-cases

Even though it tests nothing.

src/domain_tests/Python.DomainReloadTests.15.csproj

src/runtime/Python.Runtime.csproj

src/runtime/maybeserialize.cs

lostmsu · 2020-11-19T21:59:23Z

src/runtime/maybeserialize.cs

+                string[] types = new string[p.Length];
+                for (int i = 0; i < p.Length; i++)
+                {
+                    types[i] = p[i].ParameterType.AssemblyQualifiedName;


Does this work correctly for out and ref parameters? There might be more edge cases.

Potentially in parameters also.

I'll have to test

Did a quick check and out and ref parameters both reflect to void function (SomeType ByRef) with the type argument being of type (SomeType&, ...). Haven't been able to test with in (couldn't get to set the language version high enough in TestRunner.cs), but it looks like it's doing the correct thing. Will add a test case for it.

Ah, found an edge case where changing ref to out will still "resolve" the function even though the semantics are now different

lostmsu · 2020-11-19T22:05:36Z

src/runtime/moduleobject.cs

+            // Trying to remove a key that's not in the dictionary may 
+            // raise an error. We don't care about it.
+            Runtime.PyErr_Clear();


You must check for the specific error type after each call. See PyObject.GetAttrOrDefault for example.

lostmsu · 2020-11-19T22:07:34Z

src/runtime/typemanager.cs

+            var _cache = new Dictionary<MaybeType, IntPtr>();
+            storage.GetValue("cache", out _cache);


the value you put into _cache here will always be overwritten.

You can (and should) use out var _cache.

lostmsu · 2020-11-19T22:08:38Z

src/runtime/typemanager.cs

+                catch
+                {
+                    Runtime.XDecref(entry.Value);
+                    continue;
+                }


Is there a specific exception we could handle?
Can the exception be avoided in the first place by adding a precheck?

... adding a precheck?

Yes

src/runtime/metatype.cs

Serialization of System.Type, MemberInfo and MethodBase is now string based. At deserialization, use reflection to attempt to recreate the object, which may fail safely instead of throwing a SerializaitonException during the deserialization of the whoie data stream. Appropriate Exceptions will now be raised when the Maybe*'s Value property. ClasseBase objects are now de-initialized and re-initialized in Reload mode so that it's tp_dict picks up newly added members and removed members no longer linger. ModuleObject clears it's cache and remove cached members from it's tp_dict. Minor refactoring and modernization of MethodObject and MethodBinder

Changing a type's attribute causes problem with it's cache. Force the type to refresh itself when modifying it.

* Revert line endings change in Python.Runtime.csproj * Split maybe serialize into respective class files * Name changes for consistency

codecov-io · 2020-11-23T16:45:51Z

Codecov Report

Merging #1287 (79516f1) into master (f8c27a1) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master    #1287   +/-   ##
=======================================
  Coverage   74.04%   74.04%           
=======================================
  Files           1        1           
  Lines         289      289           
=======================================
  Hits          214      214           
  Misses         75       75

Flag	Coverage Δ
setup_windows	`74.04% <ø> (ø)`

Flags with carried forward coverage won't be shown. Click here to find out 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 9307bb3...5c14aad. Read the comment docs.

So that we can use that same logic when deserializing Maybe* types

BadSingleton · 2020-11-23T18:31:57Z

src/runtime/StateSerialization/MaybeMemberInfo.cs

+        // based on it's setter/getter (which is a method 
+        //  info) visibility and events based on their
+        // AddMethod visibility.
+        static bool ShouldBindMember(MemberInfo mi)


I'm not totally satisfied with this solution, but it's the best I could find. If anyone has a better idea..

You could also leave this abstract and have class MaybePropertyInfo : MaybeMemberInfo<PropertyInfo> and so on, enforcing that if the type of member changed then it doesn't count as a match.

But maybe that's not critical.

Also add a test

Serialization of System.Type, MemberInfo and MethodBase is now string based. At deserialization, use reflection to attempt to recreate the object, which may fail safely instead of throwing a SerializaitonException during the deserialization of the whoie data stream. Appropriate Exceptions will now be raised when the Maybe*'s Value property. ClasseBase objects are now de-initialized and re-initialized in Reload mode so that it's tp_dict picks up newly added members and removed members no longer linger. ModuleObject clears it's cache and remove cached members from it's tp_dict. Minor refactoring and modernization of MethodObject and MethodBinder

Changing a type's attribute causes problem with it's cache. Force the type to refresh itself when modifying it.

* Revert line endings change in Python.Runtime.csproj * Split maybe serialize into respective class files * Name changes for consistency

So that we can use that same logic when deserializing Maybe* types

Because it can't find the python library

…ypes themselves)

Because tp_clear sets tpHandle to NULL, it can't be used. Fortunately, we can simply read object's type from pyHandle.

… engine shutdown (pythonnet#1260) pythonnet#1256 pythonnet#1256 During engine shutdown all links from Python to .NET instances are severed. If an instance of CLR class defined in Python survives the shutdown (for example, a reference is stored in static field) and later gets finalized, it will attempt to severe link again, which is an invalid operation. The fix is to check if the link has already been severed and skip that step during finalization.

BadSingleton · 2020-12-18T19:02:12Z

Build failures are due to NuGet hiccups


  /usr/share/dotnet/sdk/5.0.101/NuGet.targets(131,5): error : Failed to retrieve information about 'System.Security.Permissions' from remote source 'https://api.nuget.org/v3-flatcontainer/system.security.permissions/index.json'. [/tmp/pip-req-build-49umrav0/src/runtime/Python.Runtime.csproj]
  /usr/share/dotnet/sdk/5.0.101/NuGet.targets(131,5): error :   Response status code does not indicate success: 503 (Service Not Available). [/tmp/pip-req-build-49umrav0/src/runtime/Python.Runtime.csproj]

src/domain_tests/Python.DomainReloadTests.csproj

lostmsu · 2020-12-18T20:34:48Z

src/domain_tests/TestRunner.cs

+    }}
+}}
+";
+        readonly static string PythonDllLocation = Path.Combine(AppDomain.CurrentDomain.BaseDirectory, "Python.Runtime.dll");


NIT: I might be more reliable to do typeof(Py).Assembly.Location.

src/domain_tests/TestRunner.cs

lostmsu · 2020-12-18T20:38:29Z

src/domain_tests/TestRunner.cs

+                File.Delete(tempFolderPython);
+            }
+
+            File.Copy(PythonDllLocation, tempFolderPython);


IMHO, the test should also delete the DLL.
I would also create a new folder (and delete after) for every run instead of always putting DLL directly in %TEMP%, because this might affect tests running in parallel.

I totally agree with this

lostmsu · 2020-12-18T20:39:56Z

src/domain_tests/TestRunner.cs

+            CompilerParameters parameters = new CompilerParameters();
+            parameters.GenerateExecutable = exe;
+            var assemblyName = name;
+            var assemblyFullPath = Path.Combine(Path.GetTempPath(), assemblyName);


Same: better have a run-specific folder.

src/runtime/metatype.cs

lostmsu · 2020-12-18T21:25:44Z

src/runtime/methodbinder.cs

+            if (mi == null)
+            {
+                return -1;
+            }
+


I think int.Max would be more appropriate. Higher the number - lower the priority.

UPD. can this even be null?

it could be null if we change the return type to int? but it's not useful. int.Max it is

lostmsu · 2020-12-18T21:29:02Z

src/runtime/methodobject.cs

-        }
-
-        private void _MethodObject(Type type, string name, MethodInfo[] info)
+        //  `allow_threads = true`: True being the default value of MethodBinder.allow_threads


Better use a constant.

src/runtime/moduleobject.cs

src/runtime/runtime.cs

BadSingleton · 2020-12-18T22:20:29Z

I'll be on holidays for two weeks, I'll fix that when back in office

lostmsu · 2021-01-04T21:53:41Z

src/domain_tests/TestRunner.cs

+
+        static void SetupTestFolder(string testCaseName)
+        {
+            TestPath = Path.Combine(Path.GetTempPath(), $"Python.TestRunner.{testCaseName}");


I think some random number should also be added to indicate the overall run, so that different runs would not clash.

They wouldn't, if the folder exists when setting up the test, it gets deleted first. Also, the executable can only run one test per invocation. One could run all the tests in parallel, and there'd be no clashing. If a test fails, the folder is not deleted and the contents cans be examined.

I still see a problem with running tests for multiple Python versions on the same machine in parallel.

Ah, I hadn't thought about that one. I'll add the python version and architecture to the folder name.

I'll add the PID instead

…t-cases-fixes

To the test folder name

BadSingleton added 6 commits November 9, 2020 15:25

Adds test cases for member changes during a domain reload

fc7da1d

The tests are marked as expected failures for now.

!fixup add the project to the solution files

72fafdd

Add more test cases

20861b2

Merge remote-tracking branch 'upstream/master' into domain-reload-tes…

2253ef3

…t-cases

Make the tests run as netcoreapp

635edac

Even though it tests nothing.

Remove stray colon

0ee931e

lostmsu requested changes Nov 19, 2020

View reviewed changes

BadSingleton force-pushed the domain-reload-test-cases-fixes branch from 946107f to 3dad96e Compare November 20, 2020 17:17

BadSingleton added 6 commits November 20, 2020 15:37

Call PyType_Modified after modifying the type

90a81f3

Changing a type's attribute causes problem with it's cache. Force the type to refresh itself when modifying it.

Some code review changes

10276f1

* Revert line endings change in Python.Runtime.csproj * Split maybe serialize into respective class files * Name changes for consistency

fixup! Some code review changes

4d0e2ce

Remove hungarian notation from Maybe* types

02fa245

Check the type of the exception before ignoring it

91c881c

Check for validity, don't throw

284e8e1

BadSingleton added 2 commits November 23, 2020 12:18

Refactor the member binding logic of ClassManager

fe96781

So that we can use that same logic when deserializing Maybe* types

Include info about why deserialization failed in Maybe*

6ff9e0b

BadSingleton commented Nov 23, 2020

View reviewed changes

BadSingleton added 11 commits November 24, 2020 13:14

improve deserialization resolution of ref, out and in parameters

4d2d05b

Also add a test

Clean up the project and solution files

5f061bc

Merge branch 'master' into domain-reload-test-cases

3adc559

Call PyType_Modified after modifying the type

61b0d8c

Changing a type's attribute causes problem with it's cache. Force the type to refresh itself when modifying it.

Some code review changes

c8bacf3

* Revert line endings change in Python.Runtime.csproj * Split maybe serialize into respective class files * Name changes for consistency

fixup! Some code review changes

cde5c23

Remove hungarian notation from Maybe* types

a956773

Check the type of the exception before ignoring it

ee3b391

Check for validity, don't throw

9b4d5f9

Refactor the member binding logic of ClassManager
a2f3294
So that we can use that same logic when deserializing Maybe* types

BadSingleton and others added 9 commits December 18, 2020 13:45

Test runner docs fixes

59e81e2

Skip the domain reload tests on macos

1383b5a

Because it can't find the python library

fixup! Skip the domain reload tests on macos

fbc06ef

Update ISSUE_TEMPLATE.md

b3e86da

refactoring in CreateSubType

0b027c6

allocate space for GCHandle in instances of CLR Metatype (which are t…

b4533c4

…ypes themselves)

classderived: handle tp_dealloc called after tp_clear

0a3f044

Because tp_clear sets tpHandle to NULL, it can't be used. Fortunately, we can simply read object's type from pyHandle.

a few extra assertions

639236a

BadSingleton force-pushed the domain-reload-test-cases-fixes branch from 8639f09 to 3069285 Compare December 18, 2020 18:45

Merge branch 'master' into domain-reload-test-cases-fixes

5c14aad

lostmsu requested changes Dec 18, 2020

View reviewed changes

BadSingleton added 3 commits January 4, 2021 10:42

Merge branch 'master' into domain-reload-test-cases-fixes

00c19d5

Fix break introduced by merge

833e836

Code review fixes

62ae107

BadSingleton requested a review from lostmsu January 4, 2021 21:04

lostmsu reviewed Jan 4, 2021

View reviewed changes

Merge remote-tracking branch 'upstream/master' into domain-reload-tes…

eedbae5

…t-cases-fixes

BadSingleton requested a review from lostmsu January 5, 2021 16:40

BadSingleton force-pushed the domain-reload-test-cases-fixes branch from ac877c2 to 5779d5f Compare January 6, 2021 18:42

Add the PID of the test runner

73f39bc

To the test folder name

BadSingleton force-pushed the domain-reload-test-cases-fixes branch from 5779d5f to 73f39bc Compare January 6, 2021 20:04

lostmsu approved these changes Jan 7, 2021

View reviewed changes

lostmsu merged commit d6c0081 into pythonnet:master Jan 7, 2021

BadSingleton mentioned this pull request Jan 8, 2021

Add more more tests for in, out and ref parameters #1349

Merged

4 tasks

This was referenced Jan 19, 2021

What should reload reload? #1268

Closed

Soft reload throws in deserialization in a common workflow #1250

Closed

C-SELLERS mentioned this pull request Mar 4, 2021

ClassManager Cache Performance Impact #1407

Open

		var _cache = new Dictionary<MaybeType, IntPtr>();
		storage.GetValue("cache", out _cache);

Domain reload test cases fixes #1287

Domain reload test cases fixes #1287

Uh oh!

Conversation

What does this implement/fix? Explain your changes.

Does this close any currently open issues?

Any other comments?

Checklist

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!