NotificationsYou must be signed in to change notification settings
Fork748
Star5.1k

Domain reload test cases fixes#1287

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Jump to bottom

Merged

lostmsu merged 62 commits intopythonnet:masterfromUnity-Technologies:domain-reload-test-cases-fixes

Jan 7, 2021

Merged

Domain reload test cases fixes#1287

lostmsu merged 62 commits intopythonnet:masterfromUnity-Technologies:domain-reload-test-cases-fixes

Jan 7, 2021

Conversation

Copy link

Contributor

BadSingleton commentedNov 19, 2020

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 toAUTHORS
Updated theCHANGELOG

BadSingleton added6 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

src/domain_tests/Python.DomainReloadTests.15.csproj OutdatedShow resolvedHide resolved

src/runtime/Python.Runtime.csproj OutdatedShow resolvedHide resolved

src/runtime/maybeserialize.cs OutdatedShow resolvedHide resolved

src/runtime/maybeserialize.cs Outdated

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

Copy link

Member

lostmsuNov 19, 2020

Choose a reason for hiding this comment

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

Does this work correctly forout andref parameters? There might be more edge cases.

Potentiallyin parameters also.

Copy link

ContributorAuthor

BadSingletonNov 20, 2020

Choose a reason for hiding this comment

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

I'll have to test

Copy link

ContributorAuthor

BadSingletonNov 23, 2020

Choose a reason for hiding this comment

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

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

Copy link

ContributorAuthor

BadSingletonNov 24, 2020

Choose a reason for hiding this comment

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

Ah, found an edge case where changingref toout will still "resolve" the function even though the semantics are now different

src/runtime/moduleobject.cs Outdated

Comment on lines 350 to 352

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

Copy link

Member

lostmsuNov 19, 2020

Choose a reason for hiding this comment

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

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

src/runtime/typemanager.cs Outdated

Comment on lines 80 to 81

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

Copy link

Member

lostmsuNov 19, 2020

Choose a reason for hiding this comment

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

the value you put into_cache here will always be overwritten.

You can (and should) useout var _cache.

src/runtime/typemanager.cs Outdated

Comment on lines 89 to 87

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

Copy link

Member

lostmsuNov 19, 2020

Choose a reason for hiding this comment

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

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

Copy link

ContributorAuthor

BadSingletonNov 20, 2020

Choose a reason for hiding this comment

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

... adding a precheck?

Yes

src/runtime/metatype.csShow resolvedHide resolved

Rework the serialization of reflected types

3dad96e

Serialization of System.Type, MemberInfo and MethodBase is now stringbased. At deserialization, use reflection to attempt to recreate theobject, which may fail safely instead of throwing aSerializaitonException during the deserialization of the whoie datastream. Appropriate Exceptions will now be raised when the Maybe*'sValue property.ClasseBase objects are now de-initialized and re-initialized in Reloadmode so that it's tp_dict picks up newly added members and removedmembers no longer linger.ModuleObject clears it's cache and remove cached members from it'stp_dict.Minor refactoring and modernization of MethodObject and MethodBinder

BadSingleton force-pushed thedomain-reload-test-cases-fixes branch from946107f to3dad96eCompare

November 20, 2020 17:17

BadSingleton added6 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 thetype 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

Copy link

codecov-io commentedNov 23, 2020•
edited
Loading

Codecov Report

Merging#1287 (79516f1) intomaster (f8c27a1) willnot change coverage.
The diff coverage isn/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 byCodecov. Last update9307bb3...5c14aad. Read thecomment docs.

BadSingleton added2 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

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)

Copy link

ContributorAuthor

BadSingletonNov 23, 2020

Choose a reason for hiding this comment

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

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

Copy link

Contributor

benoithudsonDec 10, 2020

Choose a reason for hiding this comment

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

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

Copy link

Contributor

benoithudsonDec 10, 2020

Choose a reason for hiding this comment

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

But maybe that's not critical.

BadSingleton added11 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

Rework the serialization of reflected types

e8543cf

Serialization of System.Type, MemberInfo and MethodBase is now stringbased. At deserialization, use reflection to attempt to recreate theobject, which may fail safely instead of throwing aSerializaitonException during the deserialization of the whoie datastream. Appropriate Exceptions will now be raised when the Maybe*'sValue property.ClasseBase objects are now de-initialized and re-initialized in Reloadmode so that it's tp_dict picks up newly added members and removedmembers no longer linger.ModuleObject clears it's cache and remove cached members from it'stp_dict.Minor refactoring and modernization of MethodObject and MethodBinder

Call PyType_Modified after modifying the type

61b0d8c

Changing a type's attribute causes problem with it's cache. Force thetype 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

BadSingletonand others added9 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

fixed crash in finalizer of CLR types defined in Python, that survive…

3069285

… engine shutdown (pythonnet#1260)pythonnet#1256pythonnet#1256During 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 force-pushed thedomain-reload-test-cases-fixes branch from8639f09 to3069285Compare

December 18, 2020 18:45

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

5c14aad

Copy link

ContributorAuthor

BadSingleton commentedDec 18, 2020

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]

lostmsu requested changes

Dec 18, 2020

View reviewed changes

src/domain_tests/Python.DomainReloadTests.csproj OutdatedShow resolvedHide resolved

src/domain_tests/TestRunner.cs

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

Copy link

Member

lostmsuDec 18, 2020•
edited
Loading

Choose a reason for hiding this comment

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

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

src/domain_tests/TestRunner.cs OutdatedShow resolvedHide resolved

src/domain_tests/TestRunner.cs Outdated

		File.Delete(tempFolderPython);
		}

		File.Copy(PythonDllLocation, tempFolderPython);

Copy link

Member

lostmsuDec 18, 2020

Choose a reason for hiding this comment

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

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.

Copy link

ContributorAuthor

BadSingletonJan 4, 2021

Choose a reason for hiding this comment

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

I totally agree with this

src/domain_tests/TestRunner.cs Outdated

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

Copy link

Member

lostmsuDec 18, 2020

Choose a reason for hiding this comment

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

Same: better have a run-specific folder.

src/runtime/metatype.csShow resolvedHide resolved

src/runtime/methodbinder.cs Outdated

Comment on lines 188 to 192

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

Copy link

Member

lostmsuDec 18, 2020

Choose a reason for hiding this comment

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

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

UPD. can this even be null?

Copy link

ContributorAuthor

BadSingletonJan 4, 2021

Choose a reason for hiding this comment

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

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

src/runtime/methodobject.cs Outdated

		}

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

Copy link

Member

lostmsuDec 18, 2020

Choose a reason for hiding this comment

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

Better use a constant.

src/runtime/moduleobject.csShow resolvedHide resolved

src/runtime/runtime.csShow resolvedHide resolved

Copy link

ContributorAuthor

BadSingleton commentedDec 18, 2020

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

BadSingleton added3 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 fromlostmsu

January 4, 2021 21:04

lostmsu reviewed

Jan 4, 2021

View reviewed changes

src/domain_tests/TestRunner.cs Outdated


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

Copy link

Member

lostmsuJan 4, 2021

Choose a reason for hiding this comment

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

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

Copy link

ContributorAuthor

BadSingletonJan 5, 2021•
edited
Loading

Choose a reason for hiding this comment

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

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.