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/01/02 17:28:53 UTC
svn commit: r1819858 - /apr/apr/trunk/poll/unix/port.c
Author: ylavic
Date: Tue Jan 2 17:28:53 2018
New Revision: 1819858
URL: http://svn.apache.org/viewvc?rev=1819858&view=rev
Log:
poll, port: re-add the wakeup pipe to the pollset after it triggered.
Just like user fds (file, socket), otherwise it's one shot only (PR-61786).
Corresponding test committed in r1819857.
Modified:
apr/apr/trunk/poll/unix/port.c
Modified: apr/apr/trunk/poll/unix/port.c
URL: http://svn.apache.org/viewvc/apr/apr/trunk/poll/unix/port.c?rev=1819858&r1=1819857&r2=1819858&view=diff
==============================================================================
--- apr/apr/trunk/poll/unix/port.c (original)
+++ apr/apr/trunk/poll/unix/port.c Tue Jan 2 17:28:53 2018
@@ -358,7 +358,6 @@ static apr_status_t impl_pollset_poll(ap
unsigned int nget;
pfd_elem_t *ep;
apr_status_t rv = APR_SUCCESS;
- apr_pollfd_t fp;
*num = 0;
nget = 1;
@@ -409,32 +408,30 @@ static apr_status_t impl_pollset_poll(ap
pollset_lock_rings();
for (i = 0, j = 0; i < nget; i++) {
- fp = (((pfd_elem_t*)(pollset->p->port_set[i].portev_user))->pfd);
+ ep = (pfd_elem_t *)pollset->p->port_set[i].portev_user;
if ((pollset->flags & APR_POLLSET_WAKEABLE) &&
- fp.desc_type == APR_POLL_FILE &&
- fp.desc.f == pollset->wakeup_pipe[0]) {
+ ep->pfd.desc_type == APR_POLL_FILE &&
+ ep->pfd.desc.f == pollset->wakeup_pipe[0]) {
apr_poll_drain_wakeup_pipe(pollset->wakeup_pipe);
rv = APR_EINTR;
}
else {
- pollset->p->result_set[j] = fp;
+ pollset->p->result_set[j] = ep->pfd;
pollset->p->result_set[j].rtnevents =
get_revent(pollset->p->port_set[i].portev_events);
-
- /* If the ring element is still on the query ring, move it
- * to the add ring for re-association with the event port
- * later. (It may have already been moved to the dead ring
- * by a call to pollset_remove on another thread.)
- */
- ep = (pfd_elem_t *)pollset->p->port_set[i].portev_user;
- if (ep->on_query_ring) {
- APR_RING_REMOVE(ep, link);
- ep->on_query_ring = 0;
- APR_RING_INSERT_TAIL(&(pollset->p->add_ring), ep,
- pfd_elem_t, link);
- }
j++;
}
+ /* If the ring element is still on the query ring, move it
+ * to the add ring for re-association with the event port
+ * later. (It may have already been moved to the dead ring
+ * by a call to pollset_remove on another thread.)
+ */
+ if (ep->on_query_ring) {
+ APR_RING_REMOVE(ep, link);
+ ep->on_query_ring = 0;
+ APR_RING_INSERT_TAIL(&(pollset->p->add_ring), ep,
+ pfd_elem_t, link);
+ }
}
pollset_unlock_rings();
if ((*num = j)) { /* any event besides wakeup pipe? */
Re: svn commit: r1819858 - /apr/apr/trunk/poll/unix/port.c
Posted by Yann Ylavic <yl...@gmail.com>.
On Tue, Jan 2, 2018 at 7:11 PM, Eric Covener <co...@gmail.com> wrote:
>
> testpoll hangs at 1819857 and works at 1819861 (latest)
Thanks Eric, looks like that was it!
Re: svn commit: r1819858 - /apr/apr/trunk/poll/unix/port.c
Posted by Rainer Jung <ra...@kippdata.de>.
Hi Yann,
Am 03.01.2018 um 11:07 schrieb Yann Ylavic:
> On Wed, Jan 3, 2018 at 5:00 AM, Rainer Jung <ra...@kippdata.de> wrote:
>>
>> I did a check on Solaris 10 Sparc. Looks good as well:
>
> Thanks Rainer, the PR is about Sparc so this is quite valuable return.
> I first thought it was some concurrency issues (in mpm_event) specific
> to Solaris Sparc until I found this..
>
>>
>> - r1819856 (without the additional check and without the fix): no hang in
>> testpoll
>>
>> - r1819857 (with the additional check but still without the fix): hangs in
>> testpoll in pollset_wakeup, more precisely in the new for loop in iteration
>> 2 of 2 and there in the call to apr_pollset_wakeup().
>
> Don't you mean that the hang was in the second call to apr_pollset_poll()?
Yes, sorry.
> I don't really expect apr_pollset_wakeup() to block unless the pipe is
> full, which'd be a capacity of one byte only here...
The hang was in the following stack:
(gdb) bt full
#0 0xff0cbcb4 in _portfs () from /lib/libc.so.1
No symbol table info available.
#1 0xff046230 in port_getn () from /lib/libc.so.1
No symbol table info available.
#2 0xff374b18 in call_port_getn (port=61, list=0x1aa418, max=2,
nget=0xffbff558, timeout=-1) at .../poll/unix/port.c:110
tv = {tv_sec = 0, tv_nsec = 0}
tvptr = <optimized out>
ret = <optimized out>
rv = 0
#3 0xff374d34 in impl_pollset_poll (pollset=0x1aa3a8,
timeout=<optimized out>, num=0xffbff634, descriptors=0xffbff638)
at .../poll/unix/port.c:400
fd = <optimized out>
ret = <optimized out>
i = <optimized out>
j = <optimized out>
nget = 1
ep = <optimized out>
rv = <optimized out>
fp = <optimized out>
#4 0xff3743ac in apr_pollset_poll (pollset=0x1aa3a8, timeout=-1,
num=num@entry=0xffbff634, descriptors=descriptors@entry=0xffbff638)
at .../poll/unix/pollset.c:246
No locals.
=> So apr_pollset_poll()
#5 0x00028ffc in pollset_wakeup (tc=0xffbff6b0, data=0x0) at
.../test/testpoll.c:805
rv = <optimized out>
socket_pollfd = {p = 0xd1fd0, desc_type = APR_POLL_SOCKET,
reqevents = 1, rtnevents = 1, desc = {f = 0x1c20c0, s = 0x1c20c0},
client_data = 0x1c20c0}
pollset = 0x1aa3a8
num = 0
descriptors = 0x0
i = 1
=> So i==1, second loop iteration
#6 0x000184d0 in abts_run_test (ts=ts@entry=0xd6dd8, f=0x28f64
<pollset_wakeup>, value=value@entry=0x0) at .../test/abts.c:171
tc = {failed = 0, suite = 0xd6308}
ss = 0xd6308
#7 0x000292e4 in testpoll (suite=0xd6dd8) at .../test/testpoll.c:956
No locals.
#8 0x00046944 in main (argc=2, argv=<optimized out>) at .../test/abts.c:429
i = <optimized out>
list_provided = <optimized out>
suite = 0xd6dd8
>>
>> - r1819858 (with the additional check and with the fix): no hang in testpoll
>>
>> - r1819902 (head of trunk): no hang in testpoll
>>
>> Thanks for finding and fixing this!
>>
>> Happy new year,
>
> Best wishes to all!
>
>
> Regards,
> Yann.
Re: svn commit: r1819858 - /apr/apr/trunk/poll/unix/port.c
Posted by Yann Ylavic <yl...@gmail.com>.
On Wed, Jan 3, 2018 at 5:00 AM, Rainer Jung <ra...@kippdata.de> wrote:
>
> I did a check on Solaris 10 Sparc. Looks good as well:
Thanks Rainer, the PR is about Sparc so this is quite valuable return.
I first thought it was some concurrency issues (in mpm_event) specific
to Solaris Sparc until I found this..
>
> - r1819856 (without the additional check and without the fix): no hang in
> testpoll
>
> - r1819857 (with the additional check but still without the fix): hangs in
> testpoll in pollset_wakeup, more precisely in the new for loop in iteration
> 2 of 2 and there in the call to apr_pollset_wakeup().
Don't you mean that the hang was in the second call to apr_pollset_poll()?
I don't really expect apr_pollset_wakeup() to block unless the pipe is
full, which'd be a capacity of one byte only here...
>
> - r1819858 (with the additional check and with the fix): no hang in testpoll
>
> - r1819902 (head of trunk): no hang in testpoll
>
> Thanks for finding and fixing this!
>
> Happy new year,
Best wishes to all!
Regards,
Yann.
Re: svn commit: r1819858 - /apr/apr/trunk/poll/unix/port.c
Posted by Rainer Jung <ra...@kippdata.de>.
Am 02.01.2018 um 19:11 schrieb Eric Covener:
> On Tue, Jan 2, 2018 at 12:51 PM, Eric Covener <co...@gmail.com> wrote:
>> On Tue, Jan 2, 2018 at 12:37 PM, Yann Ylavic <yl...@gmail.com> wrote:
>>> On Tue, Jan 2, 2018 at 6:28 PM, <yl...@apache.org> wrote:
>>>> Author: ylavic
>>>> Date: Tue Jan 2 17:28:53 2018
>>>> New Revision: 1819858
>>>>
>>>> URL: http://svn.apache.org/viewvc?rev=1819858&view=rev
>>>> Log:
>>>> poll, port: re-add the wakeup pipe to the pollset after it triggered.
>>>>
>>>> Just like user fds (file, socket), otherwise it's one shot only (PR-61786).
>>>
>>> I can't really compile/test this (no Solaris at hand), reasoning based
>>> on code inspection...
>>>
>>>>
>>>> Corresponding test committed in r1819857.
>>>
>>> May someone with the {hard,soft}ware test at r1819857 and then at
>>> r1819858 (this commit) to see if it makes a difference?
>>> I expect "testpoll" to wait indefinitely before at r1819857, and work
>>> as usual at r1819858...
>>>
>>> Thanks!
>>
>> I am testing this now on Solaris/x64 and will report back. Thanks for
>> the great work on event!
>
> testpoll hangs at 1819857 and works at 1819861 (latest)
I did a check on Solaris 10 Sparc. Looks good as well:
- r1819856 (without the additional check and without the fix): no hang
in testpoll
- r1819857 (with the additional check but still without the fix): hangs
in testpoll in pollset_wakeup, more precisely in the new for loop in
iteration 2 of 2 and there in the call to apr_pollset_wakeup().
- r1819858 (with the additional check and with the fix): no hang in testpoll
- r1819902 (head of trunk): no hang in testpoll
Thanks for finding and fixing this!
Happy new year,
Rainer
Re: svn commit: r1819858 - /apr/apr/trunk/poll/unix/port.c
Posted by Eric Covener <co...@gmail.com>.
On Tue, Jan 2, 2018 at 12:51 PM, Eric Covener <co...@gmail.com> wrote:
> On Tue, Jan 2, 2018 at 12:37 PM, Yann Ylavic <yl...@gmail.com> wrote:
>> On Tue, Jan 2, 2018 at 6:28 PM, <yl...@apache.org> wrote:
>>> Author: ylavic
>>> Date: Tue Jan 2 17:28:53 2018
>>> New Revision: 1819858
>>>
>>> URL: http://svn.apache.org/viewvc?rev=1819858&view=rev
>>> Log:
>>> poll, port: re-add the wakeup pipe to the pollset after it triggered.
>>>
>>> Just like user fds (file, socket), otherwise it's one shot only (PR-61786).
>>
>> I can't really compile/test this (no Solaris at hand), reasoning based
>> on code inspection...
>>
>>>
>>> Corresponding test committed in r1819857.
>>
>> May someone with the {hard,soft}ware test at r1819857 and then at
>> r1819858 (this commit) to see if it makes a difference?
>> I expect "testpoll" to wait indefinitely before at r1819857, and work
>> as usual at r1819858...
>>
>> Thanks!
>
> I am testing this now on Solaris/x64 and will report back. Thanks for
> the great work on event!
testpoll hangs at 1819857 and works at 1819861 (latest)
Re: svn commit: r1819858 - /apr/apr/trunk/poll/unix/port.c
Posted by Eric Covener <co...@gmail.com>.
On Tue, Jan 2, 2018 at 12:37 PM, Yann Ylavic <yl...@gmail.com> wrote:
> On Tue, Jan 2, 2018 at 6:28 PM, <yl...@apache.org> wrote:
>> Author: ylavic
>> Date: Tue Jan 2 17:28:53 2018
>> New Revision: 1819858
>>
>> URL: http://svn.apache.org/viewvc?rev=1819858&view=rev
>> Log:
>> poll, port: re-add the wakeup pipe to the pollset after it triggered.
>>
>> Just like user fds (file, socket), otherwise it's one shot only (PR-61786).
>
> I can't really compile/test this (no Solaris at hand), reasoning based
> on code inspection...
>
>>
>> Corresponding test committed in r1819857.
>
> May someone with the {hard,soft}ware test at r1819857 and then at
> r1819858 (this commit) to see if it makes a difference?
> I expect "testpoll" to wait indefinitely before at r1819857, and work
> as usual at r1819858...
>
> Thanks!
I am testing this now on Solaris/x64 and will report back. Thanks for
the great work on event!
--
Eric Covener
covener@gmail.com
Re: svn commit: r1819858 - /apr/apr/trunk/poll/unix/port.c
Posted by Yann Ylavic <yl...@gmail.com>.
On Tue, Jan 2, 2018 at 6:28 PM, <yl...@apache.org> wrote:
> Author: ylavic
> Date: Tue Jan 2 17:28:53 2018
> New Revision: 1819858
>
> URL: http://svn.apache.org/viewvc?rev=1819858&view=rev
> Log:
> poll, port: re-add the wakeup pipe to the pollset after it triggered.
>
> Just like user fds (file, socket), otherwise it's one shot only (PR-61786).
I can't really compile/test this (no Solaris at hand), reasoning based
on code inspection...
>
> Corresponding test committed in r1819857.
May someone with the {hard,soft}ware test at r1819857 and then at
r1819858 (this commit) to see if it makes a difference?
I expect "testpoll" to wait indefinitely before at r1819857, and work
as usual at r1819858...
Thanks!