In message <20220627151828.45dnn3em4uv3sedi@runtux.com>,
Ralf Schlatterbeck writes:
>On Mon, Jun 27, 2022 at 02:10:06PM +0000, John Rouillard wrote:
>> 
>> John Rouillard added the comment:
>> How well have you tested your changes to make sure that the lack of
>> reinitialization can't cause security issues?
>
>I don't think this will cause security implications. Only the instance
>is cached, not the database. Note how the client calls instance.open
>with a username in Client.opendb.
>This means we do not reload all reactors and actions for each request.
>But we *do* open the database with a new user in each request.

Ok, that's what I expected from the Client initialization.

>> It looks like a new client is created on every connect which I think
>> should guarantee that database access by a client is only done with
>> proper access restrictions but the comment in
>> instance.open that says:
>> 
>>         # load the database schema
>>         # we cannot skip this part even if self.optimize is set
>>         # because the schema has security settings that must be
>>         # applied to each database instance
>> 
>> could a lack of a call to instance.open result in an incorrectly
>> applied schema being used?
>
>See above: instance.open seems to be called for each request. Of course
>we should test this, so technically the patch *is* lacking a test that
>makes sure that for each request an instance.open is called. I'm
>currently not sure how to test the wsgi in a test, though.

At the very least I can test wsgi startup using the test_liveserver.py
module. Exposing that is a mystery to me too.

>> Ralf ideas/concerns about applying this patch without tests less than
>> three weeks before the 2.2.0 release?
>
>I didn't apply that particular patch at the time because I didn't fully
>understand it (and the i18n stuff was unrelated and worked without the
>patch). But I'm fairly sure it will not cause major breakage. But we can
>put it in the next release, I'm running with intermediate versions all
>the time. Then I'll probably have experience with a big deployment for
>the next release :-)

I am considering adding this with a feature switch. Since this feature
deals with startup, I think the only place the feature switch can go
is as an argument to RequestDispatcher(). AFAIK, the invocation of
RequestDispatcher is in the wsgi wapper and is always locally coded so
this should be doable. But I am not sure if is is a good
idea/allowed.

I would change RequestDispatcher::__init__ to read:

  def __init__(self, home, debug=False, timing=False, lang=None, feature=None)

where feature is an object. So calling:

   app = RequestDispatcher(tracker_home, feature={"wsgi_single_load": true})

would set self.feature_flags and if wsgi_single_load is set would
enable Marcus' 2 code path changes. If it was not set the current code
paths would be run.

Thoughts?

This would allow yuo to use it in production but it would be disabled
unless opted into.

>> Also what is the effect of:
>> 
>>    with self.get_tracker() as tracker:
>> 
>> as a context handler in the original code? Does it do any
>> close/cleanup/release of resources?
>
>I think that was the original intention.

Does it actually clean anything up? AFAICT it doesn't right?

>> It looks like preload() just calls get_tracker(). Is it's role simply
>> to prime the pump and preload/precompile the tracker before the call
>> [...]
>This might have been the intention but I don't think this has ever
>worked (for wsgi!) because the instance is re-created on every request.
>Which means the template instantiation is *not* retained for the next
>request independent of the setting of 'optimize' in the instance.

Yeah I was always a little confused how/if optimize worked.

>You might also want to compare Marcus' change to the wsgi with the
>implementation of roundup_server in scripts/roundup_server.py: This
>*does also* cache the instance, see how the get_tracker method of
>RoundupRequestHandler caches and re-uses the instance in the TRACKERS
>(line 200). Its a little complicated how TRACKERS is only initialized in
>a sub-class but looks like the same functionality to me.

It certainly does look the same. Hence my thought to feature flag it.