Skip to content

ext/pdo: Report bound params and columns to GC - #23845

Closed
iliaal wants to merge 1 commit into
php:PHP-8.4from
iliaal:promote/pdo-getgc-boundparams
Closed

iliaal wants to merge 1 commit into
php:PHP-8.4from
iliaal:promote/pdo-getgc-boundparams

Conversation

@iliaal

@iliaal iliaal commented Sep 22, 2026 •

Copy link
Copy Markdown
Member

PDOStatement's get_gc handler never walked bound_params or bound_columns, so a reference cycle through a bindParam() or bindColumn() argument or its driver options stayed invisible to the collector. The test binds one object in each of the four slots (parameter and driver option, for both params and columns) and expects all four collected after gc_collect_cycles().

Rebased onto GH-23936, which fixed the driver_params refcount this PR also carried before.

@iliaal
iliaal force-pushed the promote/pdo-getgc-boundparams branch from fc100ce to 99f1f38 Compare September 29, 2026 15:31
@iliaal iliaal changed the title ext/pdo: Report bound params/columns to GC and fix driver_params refcount ext/pdo: Report bound params and columns to GC Sep 29, 2026
@SakiTakamachi

Copy link
Copy Markdown
Member

The SQLite tests are failing, so some changes may be needed.

PDOStatement's get_gc handler never reported bound_params or bound_columns,
so reference cycles through a bindParam()/bindColumn() variable or its
driver options stayed invisible to the collector and were never reclaimed.
Report the parameter and driver_params zvals of both tables.
@iliaal
iliaal force-pushed the promote/pdo-getgc-boundparams branch from 99f1f38 to 7a44957 Compare September 30, 2026 13:23
@iliaal

iliaal commented Sep 30, 2026

Copy link
Copy Markdown
Member Author

Windows loaded pdo_sqlite twice (Module "pdo_sqlite" is already loaded) because the test pinned the sqlite driver inside ext/pdo/tests. It now runs as a common test through PDOTest::factory().

@SakiTakamachi SakiTakamachi left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM!

@iliaal iliaal closed this in 71e56ed Sep 30, 2026
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.

2 participants