Make WordPress Core

Opened 9 years ago

Closed 8 years ago

Last modified 8 years ago

#30380 closed defect (bug) (fixed)

get_transient doesn't check the return of get_option

Reported by: jamesgol's profile jamesgol Owned by: ocean90's profile ocean90
Milestone: 4.3 Priority: low
Severity: normal Version: 2.8
Component: Options, Meta APIs Keywords: has-patch has-tests needs-testing
Focuses: performance Cc:

Description

When calling get_transient on a key that doesn't exist, notoptions[$key] is set so it won't do any further queries, but it doesn't go far enough. Since the get_option( $transient_timeout ) doesn't check the return well enough it will always cause 2 more queries. (SELECT autoload FROM wp_options ...)

If get_options( $transient_timeout ) is returning false, what is the point of trying to delete it.

Attachments (3)

patch-transient.diff (640 bytes) - added by jamesgol 9 years ago.
patch-transient-wtests.diff (2.9 KB) - added by ericmann 8 years ago.
Updated patch file, with tests
patch-transient-wtests-5.2.diff (3.0 KB) - added by ericmann 8 years ago.
Refactor the tests to remove closures (to make things PHP 5.2-compatible)

Download all attachments as: .zip

Change History (33)

#1 @johnbillion
9 years ago

  • Focuses performance added
  • Keywords has-patch needs-testing added
  • Version changed from trunk to 2.8

#2 @voldemortensen
9 years ago

This patch worked for me. I created a faux plugin that called get_transient( 'randomstuff' ) and used the query monitor before and after the patch. The two delete_option calls were indeed eliminated. Saved about 0.003 seconds.

#3 @johnbillion
9 years ago

  • Keywords 4.2-early added; needs-testing removed
  • Milestone changed from Awaiting Review to Future Release

#4 @iseulde
8 years ago

  • Milestone changed from Future Release to 4.2

has-patch 4.2-early so moving to 4.2.

This ticket was mentioned in Slack in #core by drew. View the logs.


8 years ago

#6 follow-up: @ocean90
8 years ago

The patch makes sense at first glance, but it's worth mentioning that false < time() is true, means $transient_option will be deleted from the database. Call it a clean up for broken transients. Maybe we should only avoid the delete_option( $transient_timeout ); call?

#7 @jamesgol
8 years ago

I've never seen a case where a non autoloaded option did not have a timeout. If there is an issue with that, perhaps the source of the problem should be fixed.

#8 in reply to: ↑ 6 @afercia
8 years ago

Replying to ocean90:

The patch makes sense at first glance, but it's worth mentioning that false < time() is true, means $transient_option will be deleted from the database.

This caused me some headaches while trying to understand why the featured posts in Twenty Fourteen were firing lots of queries. They've been someway improved since then see #26744. With the old implementation, my scenario was:

No posts match the featured tag, fallback to sticky posts
the transient, though intended to be autoloaded, didn't exist yet after a settings change
get_transient() will assume: "If option is not in alloptions, it is not autoloaded and thus has a timeout"
it checks if transient is in $alloptions
it is not so it will be considered a transient with expiration
get_option() will cache its non-existence in the 'notoptions' cache
get_option( $transient_timeout ) will return false
so it will check if ( false < time() ) :) which evaluates true
WordPress now thinks it has to deal with an expired transient and will try to delete it
finally, will set $value = false and return false

This caused also a second round of queries due to the old get_featured_post_ids() implementation. Have to say I've checked this some months ago, not sure about the current behavior but I fear there's some failing assumption here that doesn't cover edge cases or bad implementations.

#9 @jamesgol
8 years ago

@ocean90 If broken transients are really something to worry about then I would say something called occasionally from cron to clean them up would be better than something that can be called many times for every request.

Last edited 8 years ago by jamesgol (previous) (diff)

#10 @DrewAPicture
8 years ago

  • Keywords punt added
  • Owner set to ocean90
  • Status changed from new to reviewing

Patch still applies. @ocean90: Can you please recommend to pursue or punt for 4.2?

#11 @DrewAPicture
8 years ago

  • Keywords 4.3-early added; 4.2-early punt removed
  • Milestone changed from 4.2 to Future Release

Thinking it's too late in the cycle to mess with transients. Punting.

@ocean90: Feel free to pull this one back if you think it's OK.

This ticket was mentioned in Slack in #core by drew. View the logs.


8 years ago

#13 @obenland
8 years ago

  • Keywords 4.3-early removed
  • Milestone changed from Future Release to 4.3

#14 @MikeHansenMe
8 years ago

This patch still applies r32676

#15 @johnbillion
8 years ago

  • Keywords needs-unit-tests added

Can we get some tests on here?

@ericmann
8 years ago

Updated patch file, with tests

#16 @ericmann
8 years ago

  • Keywords has-tests added; needs-unit-tests removed

I've refreshed the patch file, and also added some tests to verify that transient options are deleted (or not deleted) under the conditions explained in the ticket.

This ticket was mentioned in Slack in #core by ericmann. View the logs.


8 years ago

#18 @obenland
8 years ago

@ocean90, do you want to look the last patch over and see if we can get that in?

#19 follow-up: @ocean90
8 years ago

  • Keywords needs-refresh added; has-tests removed
  • Priority changed from normal to low

patch-transient-wtests.diff uses closures for the tests which won't work in in PHP 5.2.
@ericmann would you like to update the tests to make them compatible with 5.2?

#20 in reply to: ↑ 19 ; follow-up: @ericmann
8 years ago

Replying to ocean90:

would you like to update the tests to make them compatible with 5.2?

Core's test framework already has closures and is thus incompatible with 5.2. Is that a hard requirement here? I think it will add unnecessary complexity to these tests and, since we aren't running tests "in production," shouldn't be tied to WP's minimum version requirements.

Last edited 8 years ago by ericmann (previous) (diff)

#21 in reply to: ↑ 20 @ocean90
8 years ago

Replying to ericmann:

Core's test framework already has closures and is thus incompatible with 5.2.

That's odd, since we're running tests against 5.2 too and they aren't failing, see https://travis-ci.org/aaronjorbin/develop.wordpress/builds/69821650.

#22 follow-up: @dd32
8 years ago

FWIW there are 4 phpunit files which are marked as PHP 5.3+ only, one which is a Closure-specific test, and the others are the Image Editors (which I can't see why are 5.3+ at first glance)

https://core.trac.wordpress.org/browser/trunk/phpunit.xml.dist#L14

If no 5.3-specific things are required, and removing it isn't difficult, tests should aim for the same support as core IMHO.

Last edited 8 years ago by dd32 (previous) (diff)

@ericmann
8 years ago

Refactor the tests to remove closures (to make things PHP 5.2-compatible)

#23 in reply to: ↑ 22 @ericmann
8 years ago

  • Keywords has-tests added; needs-refresh removed

Replying to dd32:

If no 5.3-specific things are required, and removing it isn't difficult, tests should aim for the same support as core IMHO.

I don't necessarily agree with you here, but that's an argument for a different day and venue. Updated patch attached.

#24 follow-up: @ocean90
8 years ago

[patch-transient-wtests-5.2.diff] looks good. Only add_action( 'delete_option', array( $a, 'delete_option' ) ); should be add_action( 'delete_option', array( $a, 'action' ) ); since a delete_option method doesn't exist.

Version 0, edited 8 years ago by ocean90 (next)

#25 @ocean90
8 years ago

  • Resolution set to fixed
  • Status changed from reviewing to closed

In 33110:

Transients: If get_option( $transient_timeout ) returns false, don't bother trying to delete the transient in get_transient().

props jamesgol, ericmann.
fixes #30380.

#26 in reply to: ↑ 24 @ericmann
8 years ago

Replying to ocean90:

patch-transient-wtests-5.2.diff looks good. Only add_action( 'delete_option', array( $a, 'delete_option' ) ); should be add_action( 'delete_option', array( $a, 'action' ) ); since a delete_option method doesn't exist.

Good catch. Since the action is only added to verify that it's not invoked, the tests pass either way. Thanks for catching it, though! :-)

#27 @johnbillion
8 years ago

  • Keywords needs-testing added
  • Resolution fixed deleted
  • Status changed from closed to reopened

Tests_Option_Transient::test_nonexistent_key_old_timeout() is failing on Travis. It's not failing for me locally though, when run in isolation or when running the whole suite.

This ticket was mentioned in Slack in #core by ocean90. View the logs.


8 years ago

#29 @johnbillion
8 years ago

  • Resolution set to fixed
  • Status changed from reopened to closed

In 33125:

Skip the Tests_Option_Transient::test_nonexistent_key_old_timeout() tests on multisite for now.

See #31130
Fixes #30380

#30 @ocean90
8 years ago

#23881 was marked as a duplicate.

Note: See TracTickets for help on using tickets.