[OPEN-ILS-DEV] PATCH: osrf_app_session.[ch] (miscellaneous)

[Mike Rylander]

On Wed, Dec 3, 2008 at 7:22 PM, Scott McKellar <mck9 at swbell.net> wrote:
> These patches do various things:
>

Applied to trunk, but see below for a question.

> 1. Move the declaration of osrf_app_request_struct, and its typedef as
> osrfAppRequest, out of the header and into osrf_app_session.c.
>
> 2. In the declaration of osrf_app_session_struct: remove an obsolete
> and commented-out declaration of request_queue.
>
> 3. Abolished _osrf_app_session_free(), moving its contents into
> osrfAppSessionFree().
>
> 4. In osrfAppSessionCleanup(): after freeing the cache, nullify the
> pointer to it, in the interests of good hygiene.
>
> 5. In _osrf_app_request_free(): free the messages in the result queue.
>

This is the only one I'm unsure of.  My made up possible use case is:

1: in open-ils.auth, make a request to cstore
2: get result off the queue
3: destroy the request (and the session -- we're done with cstore, so
release it)
4: process the result data

I would have thought that the result messages should be able to stand
alone from the request once the request is complete.  However(!) the
in this particular example open-ils.auth is using oilsUtilsQuickReq()
which does indeed clone off the result object from the underlying
message.  This suggests that the intention is indeed to require the
caller to copy off the result object if they want to keep it around,
allowing the request cleanup code to freely (heh) free the messages in
its queue.

Bill, can you comment on this?

--miker

> 6. In _osrf_app_request_recv(): Eliminated the useless intermediate
> variables tmp_msg used for dequeuing messages.
>
> 7. In osrf_app_session_set_locale: If the existing session_locale is
> big enough to hold the new locale, use strcpy() instead of free() and
> strdup().
>
> 8. In osrf_app_session_set_remote: If the existing remote_id is big
> enough to hold the new remote_id, use strcpy() instead of free() and
> strdup().
>
> 9. To eliminate some duplication of code, call
> osrf_app_session_set_locale() amd osrf_app_session_set_remote() in
> _osrf_app_request_recv() and osrf_app_session_reset_remote(),
> respectively.
>
> 10. Performance tweak: in osrfAppRequestRespondComplete: don't create
> the payload message unless we're actually going to use it.
>
> 11. Make osrfAppSessionCache static, since no other source file
> references it.
>
> ------------------
>
> Discussion (using the same numbering as above):
>
> 1: osrfAppRequest is an implementation detail of osrfAppSession.  No
> other source file knows anything about it, or needs to.
>
> 2: Originally, request_queue was probably a linked list.  Now it's an
> osrfHash, and has been for some time.  By now, the old obsolete
> declaration contributes nothing but clutter.
>
> 3: These two functions are effectively inseparable.  Each does stuff
> that should never be done without doing the other stuff too.  We might
> as well marry them.
>
> 5. This change plugs a potential memory leak.  Maybe there's a good
> reason why we don't free the messages in the result queue, but I
> don't see it.  Normally the queue is probably empty by the time we
> destroy the osrfAppRequest, but it doesn't cost much to check.
>
> 7 and 8: These are performance tweaks, to avoid the churning of memory.
> I suspect that most locales are the same size, and most remote_ids are
> the same size, so we should be shaving off some CPU cycles most of the
> time.  The main cost is a couple of calls to strlen(), which should be
> cheap (especially for locales, which are generally pretty short).
>
> 9: These changes not only eliminate duplication but also take advantage
> of the performance tweaks in items 7 and 8.
>
> Scott McKellar
> http://home.swbell.net/mck9/ct/
>
> Developer's Certificate of Origin 1.1 By making a contribution to
> this project, I certify that:
>
> (a) The contribution was created in whole or in part by me and I
> have the right to submit it under the open source license indicated
> in the file; or
>
> (b) The contribution is based upon previous work that, to the best
> of my knowledge, is covered under an appropriate open source license
> and I have the right under that license to submit that work with
> modifications, whether created in whole or in part by me, under the
> same open source license (unless I am permitted to submit under a
> different license), as indicated in the file; or
>
> (c) The contribution was provided directly to me by some other person
> who certified (a), (b) or (c) and I have not modified it; and
>
> (d) In the case of each of (a), (b), or (c), I understand and agree
> that this project and the contribution are public and that a record
> of the contribution (including all personal information I submit
> with it, including my sign-off) is maintained indefinitely and may
> be redistributed consistent with this project or the open source
> license indicated in the file.



-- 
Mike Rylander
 | VP, Research and Design
 | Equinox Software, Inc. / The Evergreen Experts
 | phone:  1-877-OPEN-ILS (673-6457)
 | email:  miker at esilibrary.com
 | web:  http://www.esilibrary.com