I see that we have a config duplicate function now. Switching to using that before the rest of the parsing should make this part of the problem a non issue.
The remaining part is that the duplicated session assumes that the callbacks remain valid over its lifetime, so essentially that the duplicated session will live as long (or shorter) than the original session.
Bert
Sent from Windows Mail
From: Julian Foad
Sent: Friday, December 20, 2013 3:58 PM
To: Bert Huijben
Cc: Subversion Development
Bert Huijben wrote:
> svn_ra_session_open4 already uses some variables from the config without
> duplicating them to the internal pool, so in that way it is not a first.
(It's svn_ra_open4.) That's undocumented. That sounds like a bug. Since it's already released, we should update the doc string to say so, but going forward do we want to continue that practice? I would have thought it better (less surprising) not to do so.
> svn_ra_dup_session() documents that it will re-use the original arguments
> of the first Ra session, which includes references to things like batons,
> that can't be duplicated.
That's not how I interpret what it currently says:
/**
* Open a new ra session @a *new_session to the same repository as an existing
* ra session @a old_session, copying the callbacks, auth baton, etc. from the
* old session.
Please make that doc string more explicit if the intent is that the original 'config' and other pointers must still be valid.
Also svn_ra_open4() should document the lifetime requirements of each of its arguments, and for the compound arguments ('callbacks', 'callback_baton', 'config') whether the structures themselves or only (specified) items within them need to live for the duration of the session.
> (And Ra serf already did some internal session duplication before this
> patch)
>
> No, it is not the cleanest way to implement this, but I don't see any other
> option of creating helper Ra sessions from the Ra layer for the compact shims.
>
> The way we use it now from libsvn_client is safe as we always construct these
> sessions based on the state in svn_client_ctx_t, which must outlive both Ra
> sessions.
>
> Bert
p.s. Please avoid top-posting and HTML on this list.
Thanks.
- Julian
>From: Julian Foad
>> URL: http://svn.apache.org/r1552324
>[...]
>> @@ -494,6 +494,7 @@ svn_ra_serf__open(svn_ra_session_t *sess
>>
>> serf_sess = apr_pcalloc(pool, sizeof(*serf_sess));
>> serf_sess->pool = svn_pool_create(pool);
>> + serf_sess->config = config;
>
> Is this safe -- storing the old pointer to 'config' ...
> ... and then using it later? I don't see any lifetime guarantee.
Received on 2013-12-20 16:12:35 CET