Skip to content

Transfer the ownership of method's thunk to caller(split from #958)#1003

Merged
filmor merged 8 commits intopythonnet:masterfrom
amos402:release-thunk
Dec 18, 2019
Merged

Transfer the ownership of method's thunk to caller(split from #958)#1003
filmor merged 8 commits intopythonnet:masterfrom
amos402:release-thunk

Conversation

@amos402
Copy link
Member

@amos402 amos402 commented Nov 30, 2019

What does this implement/fix? Explain your changes.

Let the caller handle the lifecycle for achieving the memory release when shutdown pythonnet regardless of normal shutdown or soft shutdown in #958.

Does this close any currently open issues?

...

Any other comments?

...

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

@codecov-io
Copy link

codecov-io commented Nov 30, 2019 •
edited
Loading

Codecov Report

Merging #1003 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #1003   +/-   ##
=======================================
  Coverage   86.71%   86.71%           
=======================================
  Files           1        1           
  Lines         301      301           
=======================================
  Hits          261      261           
  Misses         40       40
Flag Coverage Δ
#setup_linux 65.44% <ø> (ø) ⬆️
#setup_windows 71.42% <ø> (ø) ⬆️

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 c1190f4...3d2d2b6. Read the comment docs.

@lostmsu
Copy link
Member

lostmsu commented Dec 3, 2019 •
edited
Loading

Is there a reason why we are not using Marshal.GetFunctionPointerForDelegate function instead of Marshal + Thunk struct manipulation?

@amos402
Copy link
Member Author

amos402 commented Dec 4, 2019

Usually I also use Marshal.GetFunctionPointerForDelegate too. If my suspicion corrected, this historic code was written for .net1.x when Marshal.GetFunctionPointerForDelegate doesn't exists.
As for now, we can simply cover it by Marshal.GetFunctionPointerForDelegate.

@filmor
Copy link
Member

filmor commented Dec 16, 2019

@amos402 Do you think you can update this PR to make use of Marshal.GetFunctionPointerForDelegate? That would probably simplify the code quite a bit.

filmor
filmor approved these changes Dec 18, 2019
View reviewed changes
Copy link
Member

@filmor filmor left a comment

Choose a reason for hiding this comment

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

I think this can be simplified further in the future, but it's definitely better than it was before :)

@filmor filmor merged commit f0e9c38 into pythonnet:master Dec 18, 2019
AlexCatarino pushed a commit to QuantConnect/pythonnet that referenced this pull request Jun 27, 2020
@amos402 @AlexCatarino
Transfer the ownership of method's thunk to caller(split from pythonn…
9d9845d 
…et#958) (pythonnet#1003)

* Add exception helper
* Make the caller of `Interop.GetThunk` handle thunk's lifecycle(unfinished)
* Use Marshal.GetFunctionPointerForDelegate instead of Marshal + Thunk
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@filmor filmor filmor approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@amos402 @codecov-io @lostmsu @filmor