Closed Bug 1347335 Opened 3 years ago Closed 3 years ago

Lifecycle of request cache begin at apache startup

Categories

(bugzilla.mozilla.org :: General, enhancement)

Production
enhancement
Not set

Tracking

()

VERIFIED FIXED

People

(Reporter: dylan, Assigned: dylan)

References

(Blocks 1 open bug)

Details

Attachments

(1 file)

Expected Behavior:

Bugzilla->request_cache has the lifecycle of one request.
At the end of the request, we clear it.

Observed Behavior:

Bugzilla->request_cache has contents of it before each page request,
and these contents are the same every web worker.

Resolution:

This was puzzling, leading me to believe the request cache wasn't being cleared,
or the params hash or something weirder. It turns out the problem is that so long as Bugzilla->request_cache() is called during apache startup, every worker spawned after that will inherit its contents up to that point.

The correct thing to do here would be to clear the request cache
at the start of each worker, or before each request.
The former would be more ideal but is probably not compatible with plack/psgi which we hope to use some day.
I should also add this was only a problem for staging/dev as their apache size limits are set too low.
Each worker is killed after every request, so they're always starting out with a fresh (dirty) request cache.
Attached patch 1347335_1.patchSplinter Review
1. clear request cache after all cgis are compiled.
2. clear request cache before each request
3. move cleanup handler before SizeLimit.

NI: gozer, handlers get executed in the order listed, correct?
Flags: needinfo?(gozer)
Attachment #8847762 - Flags: review?(glob)
(In reply to Dylan Hardison [:dylan] from comment #2)
> NI: gozer, handlers get executed in the order listed, correct?

yes

https://perl.apache.org/docs/2.0/user/handlers/intro.html#Stacked_Handlers
PerlCleanupHandler -> RUN_ALL
"Handlers of the type RUN_ALL will be executed in the order they have been registered.."
Flags: needinfo?(gozer)
Comment on attachment 8847762 [details] [diff] [review]
1347335_1.patch

Review of attachment 8847762 [details] [diff] [review]:
-----------------------------------------------------------------

r=glob

::: mod_perl.pl
@@ +116,4 @@
>      $rl->handler($file, $file);
>  }
>  
> +Bugzilla::clear_request_cache();

a brief comment explaining why this is necessary would be useful
Attachment #8847762 - Flags: review?(glob) → review+
(In reply to Dylan Hardison [:dylan] from comment #0)
> Expected Behavior:
> 
> Bugzilla->request_cache has the lifecycle of one request.
> At the end of the request, we clear it.
> 
> Observed Behavior:
> 
> Bugzilla->request_cache has contents of it before each page request,
> and these contents are the same every web worker.
> 
> Resolution:
> 
> This was puzzling, leading me to believe the request cache wasn't being
> cleared,
> or the params hash or something weirder. It turns out the problem is that so
> long as Bugzilla->request_cache() is called during apache startup, every
> worker spawned after that will inherit its contents up to that point.

The joy of copy-on-write, this makes complete sense.

> I should also add this was only a problem for staging/dev as their apache size limits are set too low.
> Each worker is killed after every request, so they're always starting out with a fresh (dirty) request cache.

As a side-node, having Apache::SizeLimit kill workers after every request in prod is somewhat troubling... That amounts to CGI execution mode with no real benefits from caching...

> [...]
> NI: gozer, handlers get executed in the order listed, correct?

Correct!

> 1. clear request cache after all cgis are compiled.

Or disable caching during startup.

> 2. clear request cache before each request

Hoping it's not performance heavy, but a good safety/cleanliness measure

> 3. move cleanup handler before SizeLimit.

Yup.
(In reply to Philippe M. Chiasson (:gozer) from comment #5)
> As a side-node, having Apache::SizeLimit kill workers after every request in
> prod is somewhat troubling... That amounts to CGI execution mode with no
> real benefits from caching...

not in prod, in staging/dev. But also fixed now.
With comments.

To git@github.com:mozilla-bteam/bmo.git
   badbd77..7a16fee  master -> master
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.