Convert CURL resources to objects

[kocsismate]

This is a train-wreck yet... So I'd like to ask for a preliminary feedback before I start converting all the functions to use object.

[kocsismate]

@cmb69 Could you please have a look? Unfortunately, I don't understand a couple of things:

• do I have to define the get_gc handler?
• shouldn't I free the class entries in PHP_MSHUTDOWN()? I couldn't really find such precedents in other extension so my guess is that it's taken care by the engine.
• should I move initialization logic from interface.c to somewhere else (e.g. a new php_curl.c file)?
• I saw that Nikita made the xml_parser structure an object during the migration of ext/xml. Should (can) I do the same with the php_curl struct?

[cmb69]

Thanks for working on this!

• I think a get_gc handler is necessary, to make postfields available to the GC.

• The engine takes care of releasing the class entries, to my knowledge.

• interface.c is relatively large, and splitting it up may be desireable, but I think just moving the initialization and perhaps all the object stuff doesn't bring much. If we add a "object-oriented API" sometime, it would make sense to move the classes to separate files.

• Having php_curl contained curl_object saves a heap allocation, and inlining the members yields better readable code, IMO.

[kocsismate]

@cmb69 Thank you very much! I'll try to align the PR according to your suggestions :)

[kocsismate]

The PR is in much better shape now, some tests even pass locally. However, for example the ext/curl/tests/bug48203_multi.phpt test hangs indefinitely. I haven't manage to find out the root cause, but I see that the 2nd consecutive curl_init() invocation makes it behave like that.

[nikic]

As a general note, we cannot assume that ch->cp is non-null (similar for share / multi), because the object might have been created while bypassing the constructor / curl_init. Though I'm not sure how exactly to do the bypassing if we have serialization denied. Does anyone know a trick for that?

[kocsismate]

I addressed most of the existing review comments (cloning support is underway). Unfortunately, I have no idea how to trick cp to be null. What should I do with it?

[kocsismate]

Now, only 3 curl tests fail locally.

[nikic]

Don't see anything obviously wrong in the curl multi implementation.

[nikic]

I've pushed some fixed for leaks and a segfault. However, I'm not sure the fix is ideal: https://github.com/php/php-src/pull/5402/files#diff-ab7a9164033bd55887d45d0a6cb837eeR542-R548

The problem is that the multi handle holds on to the easy handles, but the easy handles may end up being freed first. The "verify handle" loop then causes use after free. We can check detect this situation, but I'm still a bit uncertain about safety. In particular, wouldn't curl_multi_cleanup still try to access the easy handles? As I'm not getting valgrind errors anymore, something must protect against that, but I'm not sure what.

[nikic]

Hm, I guess that can't happen, because curl will automatically remove the easy handle from the multi handle if the easy handle is destroyed. So this should be safe.

I think it may make sense to make curl_multi_close() not a complete no-op though, and do a loop to remove all easy handles from the multi handle in there. If cycles are involved, one would not be able to get predictable cleanup order otherwise.

[kocsismate]

@nikic Thank you very much for your efforts to fix this! I'll try to implement the suggested improvement for curl_multi_close() a bit later.

[kocsismate]

@nikic I've just added the for-loop, but I don't get why it's better than calling zend_llist_clean() directly? Or did I just misunderstand you?

Besides, undortunately, the test still fails for me with a segfault (and a memory leak). I ran ./run-tests.php -m and the .mem file shows the following (I'm copy-pasting it in the hope that it's useful):

--54603-- UNKNOWN fcntl 101!
--54603-- UNKNOWN fcntl 101! (repeated 2 times)
--54603-- UNKNOWN fcntl 101! (repeated 4 times)
--54603-- UNKNOWN fcntl 101! (repeated 8 times)
--54603-- UNKNOWN fcntl 101! (repeated 16 times)
--54603-- UNKNOWN fcntl 101! (repeated 32 times)
--54603-- UNKNOWN fcntl 101! (repeated 64 times)
--54603-- UNKNOWN fcntl 101! (repeated 128 times)
==54603== 
==54603== Process terminating with default action of signal 4 (SIGILL)
==54603==  Illegal opcode at address 0x1042A1A73
==54603==    at 0x1042A1A73: __pthread_init.cold.2 (in /usr/lib/system/libsystem_pthread.dylib)
==54603==    by 0x10137472D: libSystem_initializer (in /usr/lib/libSystem.B.dylib)
==54603==    by 0x100DB01E2: ImageLoaderMachO::doModInitFunctions(ImageLoader::LinkContext const&) (in /usr/lib/dyld)
==54603==    by 0x100DB05ED: ImageLoaderMachO::doInitialization(ImageLoader::LinkContext const&) (in /usr/lib/dyld)
==54603==    by 0x100DAB00A: ImageLoader::recursiveInitialization(ImageLoader::LinkContext const&, unsigned int, char const*, ImageLoader::InitializerTimingList&, ImageLoader::UninitedUpwards&) (in /usr/lib/dyld)
==54603==    by 0x100DAAF75: ImageLoader::recursiveInitialization(ImageLoader::LinkContext const&, unsigned int, char const*, ImageLoader::InitializerTimingList&, ImageLoader::UninitedUpwards&) (in /usr/lib/dyld)
==54603==    by 0x100DA9013: ImageLoader::processInitializers(ImageLoader::LinkContext const&, unsigned int, ImageLoader::InitializerTimingList&, ImageLoader::UninitedUpwards&) (in /usr/lib/dyld)
==54603==    by 0x100DA90B3: ImageLoader::runInitializers(ImageLoader::LinkContext const&, ImageLoader::InitializerTimingList&) (in /usr/lib/dyld)
==54603==    by 0x100D9759F: dyld::initializeMainExecutable() (in /usr/lib/dyld)
==54603==    by 0x100D9CAF7: dyld::_main(macho_header const*, unsigned long, int, char const**, char const**, char const**, unsigned long*) (in /usr/lib/dyld)
==54603==    by 0x100D96226: dyldbootstrap::start(dyld3::MachOLoaded const*, int, char const**, dyld3::MachOLoaded const*, unsigned long*) (in /usr/lib/dyld)
==54603==    by 0x100D96024: _dyld_start (in /usr/lib/dyld)

[nikic]

Looks like there's an assertion failure when running guzzle tests:

80) GuzzleHttp\Tests\PoolTest::testExecutesPendingWhenWaiting
php: /home/nikic/php-src/Zend/zend_types.h:1147: zend_gc_delref: Assertion `p->refcount > 0' failed.
Aborted

[kocsismate]

@nikic Thank you very much for the help! I have a good friend at Guzzle, I'll talk to him about the migration. :) What's the problem with the last commit?

P.S: I've run the regular tests locally, and the ext/curl/tests/bug76675.phpt test still fails. I was trying to hunt down the issue in the previous days, but I've not had success yet.

[kocsismate]

Ah, sorry, I thought that the last commit was yours. But in fact, it was mine. That said, I'll revert it.

[nikic]

P.S: I've run the regular tests locally, and the ext/curl/tests/bug76675.phpt test still fails. I was trying to hunt down the issue in the previous days, but I've not had success yet.

The valgrind log you posted doesn't really look related. Just to double check, it does not occur on master when running under valgrind?

[nikic]

I pushed a fix for the last commit, so no need to revert anymore.

[kocsismate]

it does not occur on master when running under valgrind?

@nikic Haha, you're right, it occurs :) If I knew this before that also it fails on master...

[carusogabriel]

@kocsismate Mind to label these PRs with resources or something, the same way we are doing with stubs? That will help me organize some stuff for PHP 8.0 presentations

[kocsismate]

@carusogabriel Unfortunately, I can't create new labels.

[gmponos]

btw is this merged.. ? I can see that it is, right? Not asking in bad way.. but doesn't this need an RFC? Maybe one exists but I can't find it here..

[kocsismate]

@gmponos It's merged. See the comment from php-pulls (b516566).

This doesn't need an RFC because resources are a legacy data structure which should be eliminated. Since there is no resource type declaration, this change is mostly backward compatible (not counting the is_resource() checks and a few smaller things).

[twose]

Hi @kocsismate @nikic
We have rewritten curl as an object to support the coroutine feature, and I think that some community feedback we collected can be shared with you here

1. is_resource()
2. (int) $curl
3. (string) $curl (I didn’t see you talking about it)

The above problems have caused many community components down

Solving the backward compatibility problem of is_resource is the most difficult, I also want to know how you plan to solve it (It is not possible to do that at the extension layer, is_rousure is kind of inline)

I'm not fond of the idea (fix them) too, but I am obliged to admit the fact that BC break has a huge impact on the PHP community

[nikic]

@twose The situation for us is probably a bit different than for swoole. Swoole is at a bit of a disadvantage here, because most libraries will not be willing to make changes, even very simple changes, to make code work with swoole, if it works with normal PHP. On the other hand, they don't have much choice when it comes to PHP support :) Of the most important curl consumers, composer did not require updates, Symfony is already compatible with objects and Guzzle is being made compatible in guzzle/guzzle#2715. Of course, there is a long tail of less significant libraries. But at least based on the examples we have, the effort to be compatible with PHP 8 is quite minimal, as far as curl is concerned.

[GrahamCampbell]

Note that that PR is somewhat in complete. All phpdoc saying "resource" will need to be careful checked. Because there are no resource types in the language, we won't have errors shown to us by runtime typing.

[nikic]

@GrahamCampbell Changes to phpdoc type hints are not relevant to this discussion, because that's something we cannot mitigate from the php-src side. You'll want to talk with the phpstan maintainer on how you can avoid false-positive warnings, or just disable checks.

[twose]

@nikic You're right, PHP kernel changes will force the community to make changes, it's good for us too, Thank you for your hard work :)
BTW, I have never understood why there is a resource type before, I recently knew that it's a historical burden...