You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@apr.apache.org by tr...@apache.org on 2015/04/05 15:00:33 UTC
svn commit: r1671390 - in /apr/apr/branches/1.6.x: ./ poll/unix/poll.c
Author: trawick
Date: Sun Apr 5 13:00:33 2015
New Revision: 1671390
URL: http://svn.apache.org/r1671390
Log:
Merge r1671389 from trunk:
poll() implementation of apr_pollset_poll(): Return APR_EINTR as appropriate.
(APR_SUCCESS was returned instead in that scenario.)
Modified:
apr/apr/branches/1.6.x/ (props changed)
apr/apr/branches/1.6.x/poll/unix/poll.c
Propchange: apr/apr/branches/1.6.x/
------------------------------------------------------------------------------
--- svn:mergeinfo (original)
+++ svn:mergeinfo Sun Apr 5 13:00:33 2015
@@ -1,4 +1,4 @@
/apr/apr/branches/1.4.x:1003369,1101301
-/apr/apr/trunk:733052,739635,741862,741866-741867,741869,741871,745763-745764,746310,747990,748080,748361,748371,748565,748888,748902,748988,749810,760443,767895,775683,782838,783398,783958,784633,784773,788588,789050,793192-793193,794118,794485,795267,799497,800627,809745,809854,810472,811455,813063,821306,829490,831641,832904,835607,888669,892028,892159,892435,892909,896382,896653,908427,910419,910597,917819,917837-917838,925965,929796,931973,951771,960665,960671,979891,983618,989450,990435,1003338,1044440,1044447,1055657,1072165,1078845,1081462,1081495,1083038,1083242,1084662,1086695,1088023,1089031,1089129,1089438,1099348,1103310,1183683,1183685-1183686,1183688,1183693,1183698,1213382,1235047,1236970,1237078,1237507,1240472,1340286,1340288,1340470,1341193,1341196,1343233,1343243,1367050,1368819,1370494,1372018,1372022,1372093,1372849,1376957,1384764,1389077,1400200,1402868,1405985,1406690,1420106,1420109,1425356,1428809,1438940,1438957-1438959,1442903,1449568,1456418,1459994,146
0179-1460180,1460241,1460399,1460405,1462738,1462813,1470186,1470348,1475509,1478905,1480067,1481262,1481265,1484271,1487796,1489517,1496407,1502804,1510354,1516261,1523384,1523479,1523484,1523505,1523521,1523604,1523613,1523615,1523844-1523845,1523853,1524014,1524031,1528797,1528809,1529488,1529495,1529515,1529521,1529668,1530786,1530800,1530988,1531554,1531768,1531884,1532022,1533104,1533111,1533979,1535027,1535157,1536744,1538171,1539374,1539389,1539455,1539603,1541054,1541061,1541486,1541655,1541666,1541744,1542601,1542779,1543033,1543056,1548575,1550907,1551650,1551659,1558905,1559382,1559873,1559975,1561040,1561260,1561265,1561321,1561347,1561356,1561361,1561394,1561555,1571894,1575509,1578420,1587045,1587063,1587543,1587545,1588878,1588937,1593611,1593614-1593615,1593680,1594684,1594708,1595549,1597797,1597803,1604590,1604596,1604598,1605104,1610854,1611023,1611107,1611110,1611117,1611120,1611125,1611184,1611193,1611466,1611515,1611517,1625173,1626564,1634615,1642159,1648830,
1664406,1664447,1664451,1664471,1664769-1664770,1664775,1664904,1664911,1664958,1666341,1666411,1666458,1666611,1667914-1667916,1671329,1671356
+/apr/apr/trunk:733052,739635,741862,741866-741867,741869,741871,745763-745764,746310,747990,748080,748361,748371,748565,748888,748902,748988,749810,760443,767895,775683,782838,783398,783958,784633,784773,788588,789050,793192-793193,794118,794485,795267,799497,800627,809745,809854,810472,811455,813063,821306,829490,831641,832904,835607,888669,892028,892159,892435,892909,896382,896653,908427,910419,910597,917819,917837-917838,925965,929796,931973,951771,960665,960671,979891,983618,989450,990435,1003338,1044440,1044447,1055657,1072165,1078845,1081462,1081495,1083038,1083242,1084662,1086695,1088023,1089031,1089129,1089438,1099348,1103310,1183683,1183685-1183686,1183688,1183693,1183698,1213382,1235047,1236970,1237078,1237507,1240472,1340286,1340288,1340470,1341193,1341196,1343233,1343243,1367050,1368819,1370494,1372018,1372022,1372093,1372849,1376957,1384764,1389077,1400200,1402868,1405985,1406690,1420106,1420109,1425356,1428809,1438940,1438957-1438959,1442903,1449568,1456418,1459994,146
0179-1460180,1460241,1460399,1460405,1462738,1462813,1470186,1470348,1475509,1478905,1480067,1481262,1481265,1484271,1487796,1489517,1496407,1502804,1510354,1516261,1523384,1523479,1523484,1523505,1523521,1523604,1523613,1523615,1523844-1523845,1523853,1524014,1524031,1528797,1528809,1529488,1529495,1529515,1529521,1529668,1530786,1530800,1530988,1531554,1531768,1531884,1532022,1533104,1533111,1533979,1535027,1535157,1536744,1538171,1539374,1539389,1539455,1539603,1541054,1541061,1541486,1541655,1541666,1541744,1542601,1542779,1543033,1543056,1548575,1550907,1551650,1551659,1558905,1559382,1559873,1559975,1561040,1561260,1561265,1561321,1561347,1561356,1561361,1561394,1561555,1571894,1575509,1578420,1587045,1587063,1587543,1587545,1588878,1588937,1593611,1593614-1593615,1593680,1594684,1594708,1595549,1597797,1597803,1604590,1604596,1604598,1605104,1610854,1611023,1611107,1611110,1611117,1611120,1611125,1611184,1611193,1611466,1611515,1611517,1625173,1626564,1634615,1642159,1648830,
1664406,1664447,1664451,1664471,1664769-1664770,1664775,1664904,1664911,1664958,1666341,1666411,1666458,1666611,1667914-1667916,1671329,1671356,1671389
/apr/apr/trunk/test/testnames.c:1460405
/httpd/httpd/trunk:1604590
Modified: apr/apr/branches/1.6.x/poll/unix/poll.c
URL: http://svn.apache.org/viewvc/apr/apr/branches/1.6.x/poll/unix/poll.c?rev=1671390&r1=1671389&r2=1671390&view=diff
==============================================================================
--- apr/apr/branches/1.6.x/poll/unix/poll.c (original)
+++ apr/apr/branches/1.6.x/poll/unix/poll.c Sun Apr 5 13:00:33 2015
@@ -261,7 +261,7 @@ static apr_status_t impl_pollset_poll(ap
}
ret = poll(pollset->p->pollset, pollset->nelts, timeout);
#endif
- (*num) = ret;
+ *num = 0;
if (ret < 0) {
return apr_get_netos_error();
}
@@ -290,8 +290,7 @@ static apr_status_t impl_pollset_poll(ap
}
}
}
- if (((*num) = j) > 0)
- rv = APR_SUCCESS;
+ *num = j;
}
if (descriptors && (*num))
*descriptors = pollset->p->result_set;
Re: svn commit: r1671390 - in /apr/apr/branches/1.6.x: ./ poll/unix/poll.c
Posted by Jeff Trawick <tr...@gmail.com>.
On 04/05/2015 03:03 PM, Yann Ylavic wrote:
> On Sun, Apr 5, 2015 at 3:06 PM, Jeff Trawick <tr...@gmail.com> wrote:
>> On 04/05/2015 09:00 AM, trawick@apache.org wrote:
>>> Author: trawick
>>> Date: Sun Apr 5 13:00:33 2015
>>> New Revision: 1671390
>>>
>>> URL: http://svn.apache.org/r1671390
>>> Log:
>>> Merge r1671389 from trunk:
>>>
>>> poll() implementation of apr_pollset_poll(): Return APR_EINTR as
>>> appropriate.
>>> (APR_SUCCESS was returned instead in that scenario.)
>>
>> Can anyone look over this and +/- 1 for the 1.5.x branch?
> [...]
>>> ==============================================================================
>>> --- apr/apr/branches/1.6.x/poll/unix/poll.c (original)
>>> +++ apr/apr/branches/1.6.x/poll/unix/poll.c Sun Apr 5 13:00:33 2015
>>> @@ -261,7 +261,7 @@ static apr_status_t impl_pollset_poll(ap
>>> }
>>> ret = poll(pollset->p->pollset, pollset->nelts, timeout);
>>> #endif
>>> - (*num) = ret;
>>> + *num = 0;
>>> if (ret < 0) {
>>> return apr_get_netos_error();
>>> }
>>> @@ -290,8 +290,7 @@ static apr_status_t impl_pollset_poll(ap
>>> }
>>> }
>>> }
>>> - if (((*num) = j) > 0)
>>> - rv = APR_SUCCESS;
>>> + *num = j;
> The description of apr_pollset_poll() in apr_poll.h states that EINTR
> is only returned when "there were no signalled descriptors at the time
> of the wakeup call" (i.e. j == 0 above), whereas now both EINTR and
> *num > 0 can be returned in this case.
>
> Also, it seems that the other pollset mechanisms (epoll, kqueue, port)
> still return SUCCESS here.
Yep, my change was bogus.
>
> What is the issue with SUCCESS? Is it that we should return EINTR
> -soon or later- whenever a pollset is woken up (for the application to
> catch the case)?
> If so, maybe instead we could delay the pipe draining until next
> call(s), when no user descriptor is simultaneously ready...
> Something like the below (based on the previous code):
I think we don't do anything special. I've essentially reverted the
original commit (and added a handy comment that was present in at least
one other implementation), and now there's a testcase for this, tested
with poll and epoll on Linux. (The testcase could use restructuring to
try more tests with all available pollset implementations.)
Thanks/sorry!
>
> Index: poll/unix/poll.c
> ===================================================================
> --- poll/unix/poll.c (revision 1670476)
> +++ poll/unix/poll.c (working copy)
> @@ -279,7 +279,6 @@ static apr_status_t impl_pollset_poll(apr_pollset_
> if ((pollset->flags & APR_POLLSET_WAKEABLE) &&
> pollset->p->query_set[i].desc_type == APR_POLL_FILE &&
> pollset->p->query_set[i].desc.f ==
> pollset->wakeup_pipe[0]) {
> - apr_poll_drain_wakeup_pipe(pollset->wakeup_pipe);
> rv = APR_EINTR;
> }
> else {
> @@ -292,6 +291,8 @@ static apr_status_t impl_pollset_poll(apr_pollset_
> }
> if (((*num) = j) > 0)
> rv = APR_SUCCESS;
> + else if (rv == APR_EINTR)
> + apr_poll_drain_wakeup_pipe(pollset->wakeup_pipe);
> }
> if (descriptors && (*num))
> *descriptors = pollset->p->result_set;
> --
>
> Regards,
> Yann.
Re: svn commit: r1671390 - in /apr/apr/branches/1.6.x: ./ poll/unix/poll.c
Posted by Yann Ylavic <yl...@gmail.com>.
On Sun, Apr 5, 2015 at 3:06 PM, Jeff Trawick <tr...@gmail.com> wrote:
> On 04/05/2015 09:00 AM, trawick@apache.org wrote:
>>
>> Author: trawick
>> Date: Sun Apr 5 13:00:33 2015
>> New Revision: 1671390
>>
>> URL: http://svn.apache.org/r1671390
>> Log:
>> Merge r1671389 from trunk:
>>
>> poll() implementation of apr_pollset_poll(): Return APR_EINTR as
>> appropriate.
>> (APR_SUCCESS was returned instead in that scenario.)
>
>
> Can anyone look over this and +/- 1 for the 1.5.x branch?
[...]
>> ==============================================================================
>> --- apr/apr/branches/1.6.x/poll/unix/poll.c (original)
>> +++ apr/apr/branches/1.6.x/poll/unix/poll.c Sun Apr 5 13:00:33 2015
>> @@ -261,7 +261,7 @@ static apr_status_t impl_pollset_poll(ap
>> }
>> ret = poll(pollset->p->pollset, pollset->nelts, timeout);
>> #endif
>> - (*num) = ret;
>> + *num = 0;
>> if (ret < 0) {
>> return apr_get_netos_error();
>> }
>> @@ -290,8 +290,7 @@ static apr_status_t impl_pollset_poll(ap
>> }
>> }
>> }
>> - if (((*num) = j) > 0)
>> - rv = APR_SUCCESS;
>> + *num = j;
The description of apr_pollset_poll() in apr_poll.h states that EINTR
is only returned when "there were no signalled descriptors at the time
of the wakeup call" (i.e. j == 0 above), whereas now both EINTR and
*num > 0 can be returned in this case.
Also, it seems that the other pollset mechanisms (epoll, kqueue, port)
still return SUCCESS here.
What is the issue with SUCCESS? Is it that we should return EINTR
-soon or later- whenever a pollset is woken up (for the application to
catch the case)?
If so, maybe instead we could delay the pipe draining until next
call(s), when no user descriptor is simultaneously ready...
Something like the below (based on the previous code):
Index: poll/unix/poll.c
===================================================================
--- poll/unix/poll.c (revision 1670476)
+++ poll/unix/poll.c (working copy)
@@ -279,7 +279,6 @@ static apr_status_t impl_pollset_poll(apr_pollset_
if ((pollset->flags & APR_POLLSET_WAKEABLE) &&
pollset->p->query_set[i].desc_type == APR_POLL_FILE &&
pollset->p->query_set[i].desc.f ==
pollset->wakeup_pipe[0]) {
- apr_poll_drain_wakeup_pipe(pollset->wakeup_pipe);
rv = APR_EINTR;
}
else {
@@ -292,6 +291,8 @@ static apr_status_t impl_pollset_poll(apr_pollset_
}
if (((*num) = j) > 0)
rv = APR_SUCCESS;
+ else if (rv == APR_EINTR)
+ apr_poll_drain_wakeup_pipe(pollset->wakeup_pipe);
}
if (descriptors && (*num))
*descriptors = pollset->p->result_set;
--
Regards,
Yann.
Re: svn commit: r1671390 - in /apr/apr/branches/1.6.x: ./ poll/unix/poll.c
Posted by Jeff Trawick <tr...@gmail.com>.
On 04/05/2015 09:00 AM, trawick@apache.org wrote:
> Author: trawick
> Date: Sun Apr 5 13:00:33 2015
> New Revision: 1671390
>
> URL: http://svn.apache.org/r1671390
> Log:
> Merge r1671389 from trunk:
>
> poll() implementation of apr_pollset_poll(): Return APR_EINTR as appropriate.
> (APR_SUCCESS was returned instead in that scenario.)
Can anyone look over this and +/- 1 for the 1.5.x branch?
>
> Modified:
> apr/apr/branches/1.6.x/ (props changed)
> apr/apr/branches/1.6.x/poll/unix/poll.c
>
> Propchange: apr/apr/branches/1.6.x/
> ------------------------------------------------------------------------------
> --- svn:mergeinfo (original)
> +++ svn:mergeinfo Sun Apr 5 13:00:33 2015
> @@ -1,4 +1,4 @@
> /apr/apr/branches/1.4.x:1003369,1101301
> -/apr/apr/trunk:733052,739635,741862,741866-741867,741869,741871,745763-745764,746310,747990,748080,748361,748371,748565,748888,748902,748988,749810,760443,767895,775683,782838,783398,783958,784633,784773,788588,789050,793192-793193,794118,794485,795267,799497,800627,809745,809854,810472,811455,813063,821306,829490,831641,832904,835607,888669,892028,892159,892435,892909,896382,896653,908427,910419,910597,917819,917837-917838,925965,929796,931973,951771,960665,960671,979891,983618,989450,990435,1003338,1044440,1044447,1055657,1072165,1078845,1081462,1081495,1083038,1083242,1084662,1086695,1088023,1089031,1089129,1089438,1099348,1103310,1183683,1183685-1183686,1183688,1183693,1183698,1213382,1235047,1236970,1237078,1237507,1240472,1340286,1340288,1340470,1341193,1341196,1343233,1343243,1367050,1368819,1370494,1372018,1372022,1372093,1372849,1376957,1384764,1389077,1400200,1402868,1405985,1406690,1420106,1420109,1425356,1428809,1438940,1438957-1438959,1442903,1449568,1456418,1459994,146
> 0179-1460180,1460241,1460399,1460405,1462738,1462813,1470186,1470348,1475509,1478905,1480067,1481262,1481265,1484271,1487796,1489517,1496407,1502804,1510354,1516261,1523384,1523479,1523484,1523505,1523521,1523604,1523613,1523615,1523844-1523845,1523853,1524014,1524031,1528797,1528809,1529488,1529495,1529515,1529521,1529668,1530786,1530800,1530988,1531554,1531768,1531884,1532022,1533104,1533111,1533979,1535027,1535157,1536744,1538171,1539374,1539389,1539455,1539603,1541054,1541061,1541486,1541655,1541666,1541744,1542601,1542779,1543033,1543056,1548575,1550907,1551650,1551659,1558905,1559382,1559873,1559975,1561040,1561260,1561265,1561321,1561347,1561356,1561361,1561394,1561555,1571894,1575509,1578420,1587045,1587063,1587543,1587545,1588878,1588937,1593611,1593614-1593615,1593680,1594684,1594708,1595549,1597797,1597803,1604590,1604596,1604598,1605104,1610854,1611023,1611107,1611110,1611117,1611120,1611125,1611184,1611193,1611466,1611515,1611517,1625173,1626564,1634615,1642159,1648830,
> 1664406,1664447,1664451,1664471,1664769-1664770,1664775,1664904,1664911,1664958,1666341,1666411,1666458,1666611,1667914-1667916,1671329,1671356
> +/apr/apr/trunk:733052,739635,741862,741866-741867,741869,741871,745763-745764,746310,747990,748080,748361,748371,748565,748888,748902,748988,749810,760443,767895,775683,782838,783398,783958,784633,784773,788588,789050,793192-793193,794118,794485,795267,799497,800627,809745,809854,810472,811455,813063,821306,829490,831641,832904,835607,888669,892028,892159,892435,892909,896382,896653,908427,910419,910597,917819,917837-917838,925965,929796,931973,951771,960665,960671,979891,983618,989450,990435,1003338,1044440,1044447,1055657,1072165,1078845,1081462,1081495,1083038,1083242,1084662,1086695,1088023,1089031,1089129,1089438,1099348,1103310,1183683,1183685-1183686,1183688,1183693,1183698,1213382,1235047,1236970,1237078,1237507,1240472,1340286,1340288,1340470,1341193,1341196,1343233,1343243,1367050,1368819,1370494,1372018,1372022,1372093,1372849,1376957,1384764,1389077,1400200,1402868,1405985,1406690,1420106,1420109,1425356,1428809,1438940,1438957-1438959,1442903,1449568,1456418,1459994,146
> 0179-1460180,1460241,1460399,1460405,1462738,1462813,1470186,1470348,1475509,1478905,1480067,1481262,1481265,1484271,1487796,1489517,1496407,1502804,1510354,1516261,1523384,1523479,1523484,1523505,1523521,1523604,1523613,1523615,1523844-1523845,1523853,1524014,1524031,1528797,1528809,1529488,1529495,1529515,1529521,1529668,1530786,1530800,1530988,1531554,1531768,1531884,1532022,1533104,1533111,1533979,1535027,1535157,1536744,1538171,1539374,1539389,1539455,1539603,1541054,1541061,1541486,1541655,1541666,1541744,1542601,1542779,1543033,1543056,1548575,1550907,1551650,1551659,1558905,1559382,1559873,1559975,1561040,1561260,1561265,1561321,1561347,1561356,1561361,1561394,1561555,1571894,1575509,1578420,1587045,1587063,1587543,1587545,1588878,1588937,1593611,1593614-1593615,1593680,1594684,1594708,1595549,1597797,1597803,1604590,1604596,1604598,1605104,1610854,1611023,1611107,1611110,1611117,1611120,1611125,1611184,1611193,1611466,1611515,1611517,1625173,1626564,1634615,1642159,1648830,
> 1664406,1664447,1664451,1664471,1664769-1664770,1664775,1664904,1664911,1664958,1666341,1666411,1666458,1666611,1667914-1667916,1671329,1671356,1671389
> /apr/apr/trunk/test/testnames.c:1460405
> /httpd/httpd/trunk:1604590
>
> Modified: apr/apr/branches/1.6.x/poll/unix/poll.c
> URL: http://svn.apache.org/viewvc/apr/apr/branches/1.6.x/poll/unix/poll.c?rev=1671390&r1=1671389&r2=1671390&view=diff
> ==============================================================================
> --- apr/apr/branches/1.6.x/poll/unix/poll.c (original)
> +++ apr/apr/branches/1.6.x/poll/unix/poll.c Sun Apr 5 13:00:33 2015
> @@ -261,7 +261,7 @@ static apr_status_t impl_pollset_poll(ap
> }
> ret = poll(pollset->p->pollset, pollset->nelts, timeout);
> #endif
> - (*num) = ret;
> + *num = 0;
> if (ret < 0) {
> return apr_get_netos_error();
> }
> @@ -290,8 +290,7 @@ static apr_status_t impl_pollset_poll(ap
> }
> }
> }
> - if (((*num) = j) > 0)
> - rv = APR_SUCCESS;
> + *num = j;
> }
> if (descriptors && (*num))
> *descriptors = pollset->p->result_set;
>
>