Skip to content

No ext/com-dotnet resources #14282

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

Merged
merged 3 commits into from
Aug 10, 2024
Merged

Conversation

nielsdos
Copy link
Member

@nielsdos nielsdos commented May 20, 2024

Note: I think that for these internal data structures, moving them away from resources has a downside: if for example a fatal error occurs during execution, then they won't be cleaned up now whereas they would've been cleaned up if they were resources.

Also need to check if the refcount can be > 0 at the end of script execution.

@kocsismate
Copy link
Member

@cmb69 In case you are interested in the resource to object migration of ext/com_dotnet

@iluuu1994
Copy link
Member

Will this still land for 8.4?

@nielsdos
Copy link
Member Author

nielsdos commented Jul 11, 2024

Will this still land for 8.4?

I don't really care about this, I mainly made this PR to help Máté further because he wasn't familiar with Windows.

@cmb69
Copy link
Member

cmb69 commented Jul 11, 2024

I'll have a look at this (probably tomorrow).

Copy link
Member

@cmb69 cmb69 left a comment

Choose a reason for hiding this comment

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

probably tomorrow

Oops.

I think that for these internal data structures, moving them away from resources has a downside: if for example a fatal error occurs during execution, then they won't be cleaned up now whereas they would've been cleaned up if they were resources.

Yeah, that is somewhat an issue, but sooner or later we need to rid resources anyway (https://wiki.php.net/rfc/resource_to_object_conversion); it seems to me the proper solution would keep track of the structures in a self made list.

But looking back at https://bugs.php.net/79332 (apparently nobody reported that issue for ~15years), and assuming that com_dotnet is rarely used nowadays, I think we can just merge this PR, and maybe add an item about a cleaner solution to the TODO list for 5.1:

* TODO: Magic __wakeup and __sleep handlers for serialization
* (can wait till 5.1) */

@nielsdos nielsdos force-pushed the no-com-dotnet-resources branch from fc2872e to 904dc63 Compare August 10, 2024 18:24
@nielsdos nielsdos marked this pull request as ready for review August 10, 2024 18:25
@nielsdos
Copy link
Member Author

Ok I rebased and updated the TODO list.
I'm not too familiar with the code and it's been a while since I looked at it, so I wasn't entirely sure that I hadn't introduced leaks of some sort (the COM objects have their own refcount). There are also not a lot of tests for com_dotnet which makes me uncomfortable too.
But if you believe this to be right then fine by me.

@cmb69
Copy link
Member

cmb69 commented Aug 10, 2024

[…] updated the TODO list.

I should have better marked my comment as irony. :) After all, this TODO comment is there for 20 years, and apparently nobody cared about it, so … But having an additional comment certainly won't hurt.

There are also not a lot of tests for com_dotnet which makes me uncomfortable too.

Indeed, that is an issue. And at least some tests require even software, which is often not available. For instance, I don't have Word, and neither has CI (and I'm not sure what's actually going on in CI; there are a couple of Zend tests which are supposed to fail, because they instantiate the com_array_proxy class, which doesn't really support this, and likely hit a segfault; unfortunately segfaults do no longer appear to be explicitly signalled by run-tests.php, although the code is still there).

But if you believe this to be right then fine by me.

I can try again with using the MS debug heap; a quick attempt yielded no issues, but that might have been a bogus attempt; I would need to actually check that this still works. ASan is likely of no help, and as far as I know, MSan is not available on Windows (neither VS nor clang toolchains).

@nielsdos
Copy link
Member Author

Doesn't break here either, but you never know ... I'll go ahead and merge it. We can add the list destruction later.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants