You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@apr.apache.org by Yann Ylavic <yl...@gmail.com> on 2017/06/02 23:02:19 UTC
Re: svn commit: r1797416 - in /apr/apr/branches/1.7.x: ./
locks/beos/proc_mutex.c locks/os2/proc_mutex.c locks/win32/proc_mutex.c
On Fri, Jun 2, 2017 at 8:37 PM, <wr...@apache.org> wrote:
> Author: wrowe
> Date: Fri Jun 2 18:37:33 2017
> New Revision: 1797416
>
> URL: http://svn.apache.org/viewvc?rev=1797416&view=rev
> Log:
> On Windows, OS2 and BEOS, the singluar lock mechanisms were already compatible
> with timed locks, so there is no delta between DEFAULT and DEFAULT_TIMED.
>
> Avoid prohibited API changes to typical OS lock information.
But these locks are also DEFAULT_TIMED, so a portable code (runnable
on these or Unixes) which like to check DEFAULT_TIMED to know whether
_timedlock() is available can't.
How about DEFAULT_TIMED = DEFAULT as we discussed already?
Re: svn commit: r1797416 - in /apr/apr/branches/1.7.x: ./
locks/beos/proc_mutex.c locks/os2/proc_mutex.c locks/win32/proc_mutex.c
Posted by William A Rowe Jr <wr...@rowe-clan.net>.
My concern is the correlation to the Unix case... Flipping values of the
_DEFAULT_TIMED constant based on the preferred timed lock implementation.
We don't create different behavior patterns between OS s.
We would better advise the dev that _DEFAULT AND _DEFAULT_TIMED cannot be
inferred from the get_ex|get_type values.
When we support set_ex on Netware these become two distinct lockmech values.
On Jun 3, 2017 10:53, "Yann Ylavic" <yl...@gmail.com> wrote:
> On Sat, Jun 3, 2017 at 2:20 AM, William A Rowe Jr <wr...@rowe-clan.net>
> wrote:
> > On Fri, Jun 2, 2017 at 6:02 PM, Yann Ylavic <yl...@gmail.com>
> wrote:
> >> On Fri, Jun 2, 2017 at 8:37 PM, <wr...@apache.org> wrote:
> >>> Author: wrowe
> >>> Date: Fri Jun 2 18:37:33 2017
> >>> New Revision: 1797416
> >>>
> >>> URL: http://svn.apache.org/viewvc?rev=1797416&view=rev
> >>> Log:
> >>> On Windows, OS2 and BEOS, the singluar lock mechanisms were already
> compatible
> >>> with timed locks, so there is no delta between DEFAULT and
> DEFAULT_TIMED.
> >>>
> >>> Avoid prohibited API changes to typical OS lock information.
> >>
> >> But these locks are also DEFAULT_TIMED, so a portable code (runnable
> >> on these or Unixes) which like to check DEFAULT_TIMED to know whether
> >> _timedlock() is available can't.
> >
> > You were changing the API behavior between 1.5.x and 1.7.x branches,
> > and that's never permissible, so the code on 1.7.x was incorrect. It
> would
> > also be awfully bad form in 2.0 to suddenly return a "different"ly
> identical
> > mutex with a different lockmech value.
>
> Agreed.
>
> >
> >> How about DEFAULT_TIMED = DEFAULT as we discussed already?
> >
> > That's a possibility, but not necessary. On the downside, that would be
> > a compile time value. On unix, you may have compiled where there is
> > _DEFAULT_TIMED of _SYSVSEM, but at runtime apr had been
> > recompiled to use _POSIX. Worse yet, you would now force _DEFAULT
> > to remain sysv if that was it's runtime identity, the entire choose-best
> > logic would be bypassed if the _DEFAULT[_TIMED] enums were no
> > longer constant.
>
> I don't get it.
>
> typedef enum {
> APR_LOCK_FCNTL, /**< fcntl() */
> APR_LOCK_FLOCK, /**< flock() */
> APR_LOCK_SYSVSEM, /**< System V Semaphores */
> APR_LOCK_PROC_PTHREAD, /**< POSIX pthread process-based locking */
> APR_LOCK_POSIXSEM, /**< POSIX semaphore process-based locking */
> APR_LOCK_DEFAULT, /**< Use the default process lock */
> #ifdef _UNIX_LIKE_
> APR_LOCK_DEFAULT_TIMED /**< Use the default process timed lock */
> #else
> APR_LOCK_DEFAULT_TIMED = APR_LOCK_DEFAULT
> #endif
> } apr_lockmech_e;
>
> is a constant thing no?
>
> >
> > You identified it yourself, the _DEFAULT is a request for the appropriate
> > mutex type, on unix, _DEFAULT is not returned for the lockmech, but the
> > actual _SYSVSEM, or _POSIX, or _FILE or whatever was used.
> >
> > Likewise _DEFAULT_TIMED means nothing on read, only on set, as
> > it is a request for a class of lockmech that is most appropriate *and*
> > is capable of _TIMED ops.
>
> Right, but why apr_os_mutex_set[_ex](DEFAULT_TIMED) should fail on
> Windows, OS2 and BEOS?
> APR_LOCK_DEFAULT_TIMED = APR_LOCK_DEFAULT allows this, and does not
> break with _get[_ex]() either I think.
>
Re: svn commit: r1797416 - in /apr/apr/branches/1.7.x: ./
locks/beos/proc_mutex.c locks/os2/proc_mutex.c locks/win32/proc_mutex.c
Posted by Yann Ylavic <yl...@gmail.com>.
On Mon, Jun 5, 2017 at 8:25 PM, William A Rowe Jr <wr...@rowe-clan.net> wrote:
> On Sat, Jun 3, 2017 at 1:13 PM, Yann Ylavic <yl...@gmail.com> wrote:
>>
>> Oh, actually you didn't change _put_ex(), I misread your commit.
>> Please ignore this ;)
>
> Having reviewed my last set of 1.6 (and 2.0 / 1.7 changes), does
> locking look to you that the code is finally ready for 1.6.2 tag?
Looks good to me yes, thanks for taking care of it.
>
> Are you satisfied with where 2.0 / 1.7 now stand?
I did not look at the changes (reverts?) on those branches yet, your
feedbacks show that there may still be some work to make the _TIMED
semantic (or some replacement) more relevant/useful...
Re: svn commit: r1797416 - in /apr/apr/branches/1.7.x: ./
locks/beos/proc_mutex.c locks/os2/proc_mutex.c locks/win32/proc_mutex.c
Posted by William A Rowe Jr <wr...@rowe-clan.net>.
On Sat, Jun 3, 2017 at 1:13 PM, Yann Ylavic <yl...@gmail.com> wrote:
>
> Oh, actually you didn't change _put_ex(), I misread your commit.
> Please ignore this ;)
Having reviewed my last set of 1.6 (and 2.0 / 1.7 changes), does
locking look to you that the code is finally ready for 1.6.2 tag?
Are you satisfied with where 2.0 / 1.7 now stand?
Re: svn commit: r1797416 - in /apr/apr/branches/1.7.x: ./
locks/beos/proc_mutex.c locks/os2/proc_mutex.c locks/win32/proc_mutex.c
Posted by Yann Ylavic <yl...@gmail.com>.
On Sat, Jun 3, 2017 at 8:07 PM, Yann Ylavic <yl...@gmail.com> wrote:
> On Sat, Jun 3, 2017 at 7:15 PM, William A Rowe Jr <wr...@rowe-clan.net> wrote:
>> On Jun 3, 2017 10:53, "Yann Ylavic" <yl...@gmail.com> wrote:
>>
>>
>> Right, but why apr_os_mutex_set[_ex](DEFAULT_TIMED) should fail on
>> Windows, OS2 and BEOS?
>> APR_LOCK_DEFAULT_TIMED = APR_LOCK_DEFAULT allows this, and does not
>> break with _get[_ex]() either I think.
>>
>>
>> How do you mean, it is allowed right now with the current implementations?
>
> APR_DECLARE(apr_status_t) apr_os_proc_mutex_put_ex(apr_proc_mutex_t **pmutex,
> apr_os_proc_mutex_t *ospmutex,
> apr_lockmech_e mech,
> int register_cleanup,
> apr_pool_t *pool);
>
> It takes a mech as argument, the previous implementation allowed to
> specify DEFAULT_TIMED not only for unixes, but also for Windows, OS2
> and BEOS.
> But now it would fail, which does not really help user code portability IMHO.
Oh, actually you didn't change _put_ex(), I misread your commit.
Please ignore this ;)
Re: svn commit: r1797416 - in /apr/apr/branches/1.7.x: ./
locks/beos/proc_mutex.c locks/os2/proc_mutex.c locks/win32/proc_mutex.c
Posted by Yann Ylavic <yl...@gmail.com>.
On Sat, Jun 3, 2017 at 7:15 PM, William A Rowe Jr <wr...@rowe-clan.net> wrote:
> On Jun 3, 2017 10:53, "Yann Ylavic" <yl...@gmail.com> wrote:
>
>
> Right, but why apr_os_mutex_set[_ex](DEFAULT_TIMED) should fail on
> Windows, OS2 and BEOS?
> APR_LOCK_DEFAULT_TIMED = APR_LOCK_DEFAULT allows this, and does not
> break with _get[_ex]() either I think.
>
>
> How do you mean, it is allowed right now with the current implementations?
APR_DECLARE(apr_status_t) apr_os_proc_mutex_put_ex(apr_proc_mutex_t **pmutex,
apr_os_proc_mutex_t *ospmutex,
apr_lockmech_e mech,
int register_cleanup,
apr_pool_t *pool);
It takes a mech as argument, the previous implementation allowed to
specify DEFAULT_TIMED not only for unixes, but also for Windows, OS2
and BEOS.
But now it would fail, which does not really help user code portability IMHO.
Re: svn commit: r1797416 - in /apr/apr/branches/1.7.x: ./
locks/beos/proc_mutex.c locks/os2/proc_mutex.c locks/win32/proc_mutex.c
Posted by William A Rowe Jr <wr...@rowe-clan.net>.
On Jun 3, 2017 10:53, "Yann Ylavic" <yl...@gmail.com> wrote:
Right, but why apr_os_mutex_set[_ex](DEFAULT_TIMED) should fail on
Windows, OS2 and BEOS?
APR_LOCK_DEFAULT_TIMED = APR_LOCK_DEFAULT allows this, and does not
break with _get[_ex]() either I think.
How do you mean, it is allowed right now with the current implementations?
Re: svn commit: r1797416 - in /apr/apr/branches/1.7.x: ./
locks/beos/proc_mutex.c locks/os2/proc_mutex.c locks/win32/proc_mutex.c
Posted by Yann Ylavic <yl...@gmail.com>.
On Sat, Jun 3, 2017 at 2:20 AM, William A Rowe Jr <wr...@rowe-clan.net> wrote:
> On Fri, Jun 2, 2017 at 6:02 PM, Yann Ylavic <yl...@gmail.com> wrote:
>> On Fri, Jun 2, 2017 at 8:37 PM, <wr...@apache.org> wrote:
>>> Author: wrowe
>>> Date: Fri Jun 2 18:37:33 2017
>>> New Revision: 1797416
>>>
>>> URL: http://svn.apache.org/viewvc?rev=1797416&view=rev
>>> Log:
>>> On Windows, OS2 and BEOS, the singluar lock mechanisms were already compatible
>>> with timed locks, so there is no delta between DEFAULT and DEFAULT_TIMED.
>>>
>>> Avoid prohibited API changes to typical OS lock information.
>>
>> But these locks are also DEFAULT_TIMED, so a portable code (runnable
>> on these or Unixes) which like to check DEFAULT_TIMED to know whether
>> _timedlock() is available can't.
>
> You were changing the API behavior between 1.5.x and 1.7.x branches,
> and that's never permissible, so the code on 1.7.x was incorrect. It would
> also be awfully bad form in 2.0 to suddenly return a "different"ly identical
> mutex with a different lockmech value.
Agreed.
>
>> How about DEFAULT_TIMED = DEFAULT as we discussed already?
>
> That's a possibility, but not necessary. On the downside, that would be
> a compile time value. On unix, you may have compiled where there is
> _DEFAULT_TIMED of _SYSVSEM, but at runtime apr had been
> recompiled to use _POSIX. Worse yet, you would now force _DEFAULT
> to remain sysv if that was it's runtime identity, the entire choose-best
> logic would be bypassed if the _DEFAULT[_TIMED] enums were no
> longer constant.
I don't get it.
typedef enum {
APR_LOCK_FCNTL, /**< fcntl() */
APR_LOCK_FLOCK, /**< flock() */
APR_LOCK_SYSVSEM, /**< System V Semaphores */
APR_LOCK_PROC_PTHREAD, /**< POSIX pthread process-based locking */
APR_LOCK_POSIXSEM, /**< POSIX semaphore process-based locking */
APR_LOCK_DEFAULT, /**< Use the default process lock */
#ifdef _UNIX_LIKE_
APR_LOCK_DEFAULT_TIMED /**< Use the default process timed lock */
#else
APR_LOCK_DEFAULT_TIMED = APR_LOCK_DEFAULT
#endif
} apr_lockmech_e;
is a constant thing no?
>
> You identified it yourself, the _DEFAULT is a request for the appropriate
> mutex type, on unix, _DEFAULT is not returned for the lockmech, but the
> actual _SYSVSEM, or _POSIX, or _FILE or whatever was used.
>
> Likewise _DEFAULT_TIMED means nothing on read, only on set, as
> it is a request for a class of lockmech that is most appropriate *and*
> is capable of _TIMED ops.
Right, but why apr_os_mutex_set[_ex](DEFAULT_TIMED) should fail on
Windows, OS2 and BEOS?
APR_LOCK_DEFAULT_TIMED = APR_LOCK_DEFAULT allows this, and does not
break with _get[_ex]() either I think.
Re: svn commit: r1797416 - in /apr/apr/branches/1.7.x: ./
locks/beos/proc_mutex.c locks/os2/proc_mutex.c locks/win32/proc_mutex.c
Posted by William A Rowe Jr <wr...@rowe-clan.net>.
On Fri, Jun 2, 2017 at 6:02 PM, Yann Ylavic <yl...@gmail.com> wrote:
> On Fri, Jun 2, 2017 at 8:37 PM, <wr...@apache.org> wrote:
>> Author: wrowe
>> Date: Fri Jun 2 18:37:33 2017
>> New Revision: 1797416
>>
>> URL: http://svn.apache.org/viewvc?rev=1797416&view=rev
>> Log:
>> On Windows, OS2 and BEOS, the singluar lock mechanisms were already compatible
>> with timed locks, so there is no delta between DEFAULT and DEFAULT_TIMED.
>>
>> Avoid prohibited API changes to typical OS lock information.
>
> But these locks are also DEFAULT_TIMED, so a portable code (runnable
> on these or Unixes) which like to check DEFAULT_TIMED to know whether
> _timedlock() is available can't.
You were changing the API behavior between 1.5.x and 1.7.x branches,
and that's never permissible, so the code on 1.7.x was incorrect. It would
also be awfully bad form in 2.0 to suddenly return a "different"ly identical
mutex with a different lockmech value.
> How about DEFAULT_TIMED = DEFAULT as we discussed already?
That's a possibility, but not necessary. On the downside, that would be
a compile time value. On unix, you may have compiled where there is
_DEFAULT_TIMED of _SYSVSEM, but at runtime apr had been
recompiled to use _POSIX. Worse yet, you would now force _DEFAULT
to remain sysv if that was it's runtime identity, the entire choose-best
logic would be bypassed if the _DEFAULT[_TIMED] enums were no
longer constant.
You identified it yourself, the _DEFAULT is a request for the appropriate
mutex type, on unix, _DEFAULT is not returned for the lockmech, but the
actual _SYSVSEM, or _POSIX, or _FILE or whatever was used.
Likewise _DEFAULT_TIMED means nothing on read, only on set, as
it is a request for a class of lockmech that is most appropriate *and*
is capable of _TIMED ops.
For each of these, perhaps _WIN32 _BEOSSEM _NWMUTEX etc.
would be more appropriate labels, because overloading those on top of
_DEFAULT led to this design confusion.
Consider that use of os_get[_ex] os_set[_ex] requires intimate knowledge
of the apr implementation and underlying mutex mechanism by platform,
so I don't see this as a major complication.