You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@apr.apache.org by yl...@apache.org on 2018/04/06 07:53:03 UTC
svn commit: r1828492 - in /apr/apr/trunk: include/apr_reslist.h
util-misc/apr_reslist.c
Author: ylavic
Date: Fri Apr 6 07:53:02 2018
New Revision: 1828492
URL: http://svn.apache.org/viewvc?rev=1828492&view=rev
Log:
reslist: follow up to r1828289: enfore empty list requirement when setting fifo.
The doxygen remark wasn't enough as noted by Ruediger.
Modified:
apr/apr/trunk/include/apr_reslist.h
apr/apr/trunk/util-misc/apr_reslist.c
Modified: apr/apr/trunk/include/apr_reslist.h
URL: http://svn.apache.org/viewvc/apr/apr/trunk/include/apr_reslist.h?rev=1828492&r1=1828491&r2=1828492&view=diff
==============================================================================
--- apr/apr/trunk/include/apr_reslist.h (original)
+++ apr/apr/trunk/include/apr_reslist.h Fri Apr 6 07:53:02 2018
@@ -136,17 +136,18 @@ APR_DECLARE(void) apr_reslist_timeout_se
apr_interval_time_t timeout);
/**
- * Set whether the reslist reuses resources as FIFO (First In First Out) or
- * LIFO (Last In First Out).
+ * Set whether the reslist should maintain resources as FIFO (First In First
+ * Out) or LIFO (Last In First Out).
* @param reslist The resource list.
* @param fifo Set as FIFO (non zero) or LIFO (zero).
- * @remark This function should be called before any resource is in the
- * the reslist, otherwise maintenance optimizations based on the expiration
- * time relative to the order of insertion (i.e. position in the list) won't
- * work as expected.
+ * @return APR_SUCCESS, or APR_EBUSY if the resource is not empty.
+ * @remark The reslist is required to be empty because some maintenance
+ * optimizations are based on the relative position of resources in the list
+ * to determine their expiry.
*
*/
-APR_DECLARE(void) apr_reslist_fifo_set(apr_reslist_t *reslist, int fifo);
+APR_DECLARE(apr_status_t) apr_reslist_fifo_set(apr_reslist_t *reslist,
+ int fifo);
/**
* Return the number of outstanding resources.
Modified: apr/apr/trunk/util-misc/apr_reslist.c
URL: http://svn.apache.org/viewvc/apr/apr/trunk/util-misc/apr_reslist.c?rev=1828492&r1=1828491&r2=1828492&view=diff
==============================================================================
--- apr/apr/trunk/util-misc/apr_reslist.c (original)
+++ apr/apr/trunk/util-misc/apr_reslist.c Fri Apr 6 07:53:02 2018
@@ -445,9 +445,15 @@ APR_DECLARE(void) apr_reslist_timeout_se
reslist->timeout = timeout;
}
-APR_DECLARE(void) apr_reslist_fifo_set(apr_reslist_t *reslist, int fifo)
+APR_DECLARE(apr_status_t) apr_reslist_fifo_set(apr_reslist_t *reslist,
+ int fifo)
{
+ if (!APR_RING_EMPTY(&reslist->avail_list, apr_res_t, link)) {
+ return APR_EBUSY;
+ }
+
reslist->fifo = fifo;
+ return APR_SUCCESS;
}
APR_DECLARE(apr_uint32_t) apr_reslist_acquired_count(apr_reslist_t *reslist)
Re: svn commit: r1828492 - in /apr/apr/trunk: include/apr_reslist.h util-misc/apr_reslist.c
Posted by Yann Ylavic <yl...@gmail.com>.
On Fri, Apr 6, 2018 at 10:26 AM, Ruediger Pluem <rp...@apache.org> wrote:
>
> On 04/06/2018 09:53 AM, ylavic@apache.org wrote:
>>
>> + if (!APR_RING_EMPTY(&reslist->avail_list, apr_res_t, link)) {
>
> Don't we need to lock if want to do this or is this sufficiently
> atomic? Further on don't we need to make the check and the setting of
> reslist->fifo atomic, hence both done while we hold the lock?
As I said in the other thread, I'd prefer to keep the test "light"
since we document how the helper should be used.
It's indeed racy without the lock but still good enough I think: only
the first added (or last removed) resource(s) concerned for that race
be relevant, and then the winner would have to pop (resp. push) the
resource(s) before the loser is rescheduled for anything to possibly
"malfunction", where the "malfunction" would be that the resource(s)
might not expire on time, while the time diff for a race is quite tiny
in the first place...
No crash/corruption possible in any case, a non-issue IMHO, and almost
deserved if one wants to use the API outside of its scope, it's not
like we enforce everything in APR :)
Regards,
Yann.
Re: svn commit: r1828492 - in /apr/apr/trunk: include/apr_reslist.h
util-misc/apr_reslist.c
Posted by Ruediger Pluem <rp...@apache.org>.
On 04/06/2018 09:53 AM, ylavic@apache.org wrote:
> Author: ylavic
> Date: Fri Apr 6 07:53:02 2018
> New Revision: 1828492
>
> URL: http://svn.apache.org/viewvc?rev=1828492&view=rev
> Log:
> reslist: follow up to r1828289: enfore empty list requirement when setting fifo.
>
> The doxygen remark wasn't enough as noted by Ruediger.
>
>
> Modified:
> apr/apr/trunk/include/apr_reslist.h
> apr/apr/trunk/util-misc/apr_reslist.c
>
>
> Modified: apr/apr/trunk/util-misc/apr_reslist.c
> URL: http://svn.apache.org/viewvc/apr/apr/trunk/util-misc/apr_reslist.c?rev=1828492&r1=1828491&r2=1828492&view=diff
> ==============================================================================
> --- apr/apr/trunk/util-misc/apr_reslist.c (original)
> +++ apr/apr/trunk/util-misc/apr_reslist.c Fri Apr 6 07:53:02 2018
> @@ -445,9 +445,15 @@ APR_DECLARE(void) apr_reslist_timeout_se
> reslist->timeout = timeout;
> }
>
> -APR_DECLARE(void) apr_reslist_fifo_set(apr_reslist_t *reslist, int fifo)
> +APR_DECLARE(apr_status_t) apr_reslist_fifo_set(apr_reslist_t *reslist,
> + int fifo)
> {
> + if (!APR_RING_EMPTY(&reslist->avail_list, apr_res_t, link)) {
Don't we need to lock if want to do this or is this sufficiently atomic?
Further on don't we need to make the check and the setting of reslist->fifo atomic, hence both done while we hold the lock?
> + return APR_EBUSY;
> + }
> +
> reslist->fifo = fifo;
> + return APR_SUCCESS;
> }
>
Regards
Rüdiger