DFS: Cache gen. timeout check only when needed

[glye]

Only check cache generation timeout when generation has actually started, to avoid SQL error.

Calling DFSHandler::checkCacheGenerationTimeout() leads to an SQL error if cache generation hasn't started. In that case, the start timestamp is false which ends up as an empty string in the mysqli query. Fix this by not checking, when the timestamp is false. Also fixes CS.

Open questions:

• Is the fact that this happens, a symptom for a bigger problem, or is it really just a long-lived bug?
• Would it be more correct to fix this within checkCacheGenerationTimeout()? Done.

[andrerom]

Looks "ok" to me, bu I don't know this part that well so maybe @bdunogier can give some insights if this is ok as well.

[bdunogier]

Is the fact that this happens, a symptom for a bigger problem, or is it really just a long-lived bug?

Yes.

+++ Divide By Cucumber Error. Please Reinstall Universe And Reboot +++

To be honest, I'm not sure I can really help there any longer. Your analysis sounds correct, @glye. Semantically speaking, it would be both more correct and more meaningful to test this in a dedicated method. Instead of adding an has, I'd maybe rather test that $this->generationStartTimestamp isn't false in checkCacheGenerationTimeout().

[glye]

Ack, moved to checkCacheGenerationTimeout() and added logging.

[gggeek]

Mmm, what if cache generation was started by another process/thread? Would any piece of code call in that scenario checkCacheGenerationTimeout without generationStartTimestamp having been set ?

[bdunogier]

Actually, I would have returned false in that case, @glye. If we don't have a timestamp, we can't store, can we ?

[gggeek]

It seems that the only entrypoint to this hairyball is towards the end of processCache(). So all of the executions should be internal to the class itself.

My question above then becomes: in which scenario does processCache call $this->storeCache (which uses the timeout value) without executing first through $this->startCacheGeneration() (which sets it) a few lines above?
I'd rather have someone take another look at the possible causes for this rather than applying this kind of blind patch...

ps: not having 'protected' and 'public' method modifiers makes control flow analysis of the codebase so much hareder... :-(

[glye]

@gggeek It should afaik be reading the start timestamp from the shared db, but there could be a race condition where this sometimes doesn't work as expected.

@bdunogier checkCacheGenerationTimeout() returns true when cache generation did not time out, false when it did (cache generating stolen). If true (not timed out) then store() may try to store the cache. It's confusing. Anyway, if generation has not started it should be safe and sensible to try storing, right? That's why I return true.

[bdunogier]

I'd rather be very protective here @glye: we are in an unidentified case (race condition is a candidate indeed). I think that in that case, it is safe to generate the result, and not even try to store it. If several try to store at the same time, that's when more races occur.

Worst case, nobody does because of the race, and it will happen later.

Worstest case, nobody ever generates it...

++?????++ Out of Cheese Error. Redo From Start.

[glye]

@gggeek Sorry, already merged before you joined in, and I'm leaving for 3 weeks. No releases likely in that time, anyway. But if you and @bdunogier agree on reverting, I'll do that.

[bdunogier]

Maybe we should revert it until we have settled the discussion ?

[gggeek]

I'll also be away from keyboard for 2 weeks :-D
Agree that on no-generation-started we should not try to store (btw what is the current behaviour? does the bad sql only generate a mysql warning or does it halt the whole page execution?).
Fine with revert...

[glye]

Reverted.