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 2021/12/29 16:53:51 UTC
svn commit: r1896510 - in /apr/apr/branches/1.7.x: ./ file_io/win32/ include/arch/unix/ include/arch/win32/ network_io/os2/ poll/os2/ poll/unix/
Author: ylavic
Date: Wed Dec 29 16:53:51 2021
New Revision: 1896510
URL: http://svn.apache.org/viewvc?rev=1896510&view=rev
Log:
Merge r1895106, r1895111, r1895175, r1895181, r1895465 from trunk:
Fix drain wakeup pipe issue when multiple threads call apr_pollset_wakeup/apr_pollcb_wakeup for the same pollset filling up drain pipe. Use atomics so that wakeup call is noop if some other thread allready done this
Follow up to r1895106: now we want blocking reads on unix too so revert r1894914.
Follow up to r1895106: Use less expensive atomics for wakeup.
If pipe writers (wakeup) put a single byte until it's consumed by the
reader (drain), it's enough to use an atomic cas for the writers (still) and
an atomic (re)set for the reader (no more cas here).
This requires that the reader never blocks on read though (e.g. spurious return
from poll), so make the read side on the pipe non-blocking again/finally.
Since synchronous non-blocking read is not a thing for Windows' Readfile(), add
a ->socket flag to this arch's apr_file_t (like the existing ->pipe one) which
file_socket_pipe_create() will set to make apr_file_read/write() handle
non-blocking (nor overlapped) socket pipes with WSARecv/Send().
Use enum instead multiple booleans
Fix remaining change when apr_filetype_e was added
Submitted by: mturk, ylavic, ylavic, mturk, mturk
Modified:
apr/apr/branches/1.7.x/ (props changed)
apr/apr/branches/1.7.x/file_io/win32/pipe.c
apr/apr/branches/1.7.x/file_io/win32/readwrite.c
apr/apr/branches/1.7.x/include/arch/unix/apr_arch_poll_private.h
apr/apr/branches/1.7.x/include/arch/win32/apr_arch_file_io.h
apr/apr/branches/1.7.x/network_io/os2/sockopt.c
apr/apr/branches/1.7.x/poll/os2/pollset.c
apr/apr/branches/1.7.x/poll/unix/epoll.c
apr/apr/branches/1.7.x/poll/unix/kqueue.c
apr/apr/branches/1.7.x/poll/unix/poll.c
apr/apr/branches/1.7.x/poll/unix/pollcb.c
apr/apr/branches/1.7.x/poll/unix/pollset.c
apr/apr/branches/1.7.x/poll/unix/port.c
apr/apr/branches/1.7.x/poll/unix/select.c
apr/apr/branches/1.7.x/poll/unix/wakeup.c
Propchange: apr/apr/branches/1.7.x/
------------------------------------------------------------------------------
Merged /apr/apr/trunk:r1895106,1895111,1895175,1895181,1895465
Modified: apr/apr/branches/1.7.x/file_io/win32/pipe.c
URL: http://svn.apache.org/viewvc/apr/apr/branches/1.7.x/file_io/win32/pipe.c?rev=1896510&r1=1896509&r2=1896510&view=diff
==============================================================================
--- apr/apr/branches/1.7.x/file_io/win32/pipe.c (original)
+++ apr/apr/branches/1.7.x/file_io/win32/pipe.c Wed Dec 29 16:53:51 2021
@@ -43,7 +43,7 @@ APR_DECLARE(apr_status_t) apr_file_pipe_
thepipe->timeout = timeout;
return APR_SUCCESS;
}
- if (!thepipe->pipe) {
+ if (thepipe->ftype != APR_FILETYPE_PIPE) {
return APR_ENOTIMPL;
}
if (timeout && !(thepipe->pOverlapped)) {
@@ -108,7 +108,7 @@ APR_DECLARE(apr_status_t) apr_file_pipe_
(*in) = (apr_file_t *)apr_pcalloc(pool_in, sizeof(apr_file_t));
(*in)->pool = pool_in;
(*in)->fname = NULL;
- (*in)->pipe = 1;
+ (*in)->ftype = APR_FILETYPE_PIPE;
(*in)->timeout = -1;
(*in)->ungetchar = -1;
(*in)->eof_hit = 0;
@@ -123,7 +123,7 @@ APR_DECLARE(apr_status_t) apr_file_pipe_
(*out) = (apr_file_t *)apr_pcalloc(pool_out, sizeof(apr_file_t));
(*out)->pool = pool_out;
(*out)->fname = NULL;
- (*out)->pipe = 1;
+ (*out)->ftype = APR_FILETYPE_PIPE;
(*out)->timeout = -1;
(*out)->ungetchar = -1;
(*out)->eof_hit = 0;
@@ -250,7 +250,7 @@ APR_DECLARE(apr_status_t) apr_os_pipe_pu
{
(*file) = apr_pcalloc(pool, sizeof(apr_file_t));
(*file)->pool = pool;
- (*file)->pipe = 1;
+ (*file)->ftype = APR_FILETYPE_PIPE;
(*file)->timeout = -1;
(*file)->ungetchar = -1;
(*file)->filehand = *thefile;
@@ -364,32 +364,28 @@ static apr_status_t create_socket_pipe(S
rv = apr_get_netos_error();
goto cleanup;
}
- /* Verify the connection by reading the send identification.
- */
- do {
- if (nc++)
- Sleep(1);
- nrd = recv(*rd, (char *)iid, sizeof(iid), 0);
- rv = nrd == SOCKET_ERROR ? apr_get_netos_error() : APR_SUCCESS;
- } while (APR_STATUS_IS_EAGAIN(rv));
-
- if (nrd == sizeof(iid)) {
- if (memcmp(uid, iid, sizeof(uid)) == 0) {
- /* Wow, we recived what we send.
- * Put read side of the pipe to the blocking
- * mode and return.
- */
- bm = 0;
- if (ioctlsocket(*rd, FIONBIO, &bm) == SOCKET_ERROR) {
- rv = apr_get_netos_error();
- goto cleanup;
- }
- break;
- }
+ /* Verify the connection by reading/waiting for the identification */
+ bm = 0;
+ if (ioctlsocket(*rd, FIONBIO, &bm) == SOCKET_ERROR) {
+ rv = apr_get_netos_error();
+ goto cleanup;
}
- else if (nrd == SOCKET_ERROR) {
+ nrd = recv(*rd, (char *)iid, sizeof(iid), 0);
+ if (nrd == SOCKET_ERROR) {
+ rv = apr_get_netos_error();
goto cleanup;
}
+ if (nrd == (int)sizeof(uid) && memcmp(iid, uid, sizeof(uid)) == 0) {
+ /* Got the right identifier, put the poll()able read side of
+ * the pipe in nonblocking mode and return.
+ */
+ bm = 1;
+ if (ioctlsocket(*rd, FIONBIO, &bm) == SOCKET_ERROR) {
+ rv = apr_get_netos_error();
+ goto cleanup;
+ }
+ break;
+ }
closesocket(*rd);
}
/* We don't need the listening socket any more */
@@ -398,6 +394,7 @@ static apr_status_t create_socket_pipe(S
cleanup:
/* Don't leak resources */
+ closesocket(ls);
if (*rd != INVALID_SOCKET)
closesocket(*rd);
if (*wr != INVALID_SOCKET)
@@ -405,7 +402,6 @@ cleanup:
*rd = INVALID_SOCKET;
*wr = INVALID_SOCKET;
- closesocket(ls);
return rv;
}
@@ -434,21 +430,21 @@ apr_status_t apr_file_socket_pipe_create
(*in) = (apr_file_t *)apr_pcalloc(p, sizeof(apr_file_t));
(*in)->pool = p;
(*in)->fname = NULL;
- (*in)->pipe = 1;
- (*in)->timeout = -1;
+ (*in)->ftype = APR_FILETYPE_SOCKET;
+ (*in)->timeout = 0; /* read end of the pipe is non-blocking */
(*in)->ungetchar = -1;
(*in)->eof_hit = 0;
(*in)->filePtr = 0;
(*in)->bufpos = 0;
(*in)->dataRead = 0;
(*in)->direction = 0;
- (*in)->pOverlapped = (OVERLAPPED*)apr_pcalloc(p, sizeof(OVERLAPPED));
+ (*in)->pOverlapped = NULL;
(*in)->filehand = (HANDLE)rd;
(*out) = (apr_file_t *)apr_pcalloc(p, sizeof(apr_file_t));
(*out)->pool = p;
(*out)->fname = NULL;
- (*out)->pipe = 1;
+ (*out)->ftype = APR_FILETYPE_SOCKET;
(*out)->timeout = -1;
(*out)->ungetchar = -1;
(*out)->eof_hit = 0;
@@ -456,7 +452,7 @@ apr_status_t apr_file_socket_pipe_create
(*out)->bufpos = 0;
(*out)->dataRead = 0;
(*out)->direction = 0;
- (*out)->pOverlapped = (OVERLAPPED*)apr_pcalloc(p, sizeof(OVERLAPPED));
+ (*out)->pOverlapped = NULL;
(*out)->filehand = (HANDLE)wr;
apr_pool_cleanup_register(p, (void *)(*in), socket_pipe_cleanup,
@@ -470,7 +466,7 @@ apr_status_t apr_file_socket_pipe_create
apr_status_t apr_file_socket_pipe_close(apr_file_t *file)
{
apr_status_t stat;
- if (!file->pipe)
+ if (file->ftype != APR_FILETYPE_SOCKET)
return apr_file_close(file);
if ((stat = socket_pipe_cleanup(file)) == APR_SUCCESS) {
apr_pool_cleanup_kill(file->pool, file, socket_pipe_cleanup);
Modified: apr/apr/branches/1.7.x/file_io/win32/readwrite.c
URL: http://svn.apache.org/viewvc/apr/apr/branches/1.7.x/file_io/win32/readwrite.c?rev=1896510&r1=1896509&r2=1896510&view=diff
==============================================================================
--- apr/apr/branches/1.7.x/file_io/win32/readwrite.c (original)
+++ apr/apr/branches/1.7.x/file_io/win32/readwrite.c Wed Dec 29 16:53:51 2021
@@ -20,10 +20,12 @@
#include "apr_strings.h"
#include "apr_lib.h"
#include "apr_errno.h"
-#include <malloc.h>
+#include "apr_arch_networkio.h"
#include "apr_arch_atime.h"
#include "apr_arch_misc.h"
+#include <malloc.h>
+
/*
* read_with_timeout()
* Uses async i/o to emulate unix non-blocking i/o with timeouts.
@@ -40,7 +42,7 @@ static apr_status_t read_with_timeout(ap
/* Peek at the pipe. If there is no data available, return APR_EAGAIN.
* If data is available, go ahead and read it.
*/
- if (file->pipe) {
+ if (file->ftype == APR_FILETYPE_PIPE) {
DWORD bytes;
if (!PeekNamedPipe(file->filehand, NULL, 0, NULL, &bytes, NULL)) {
rv = apr_get_os_error();
@@ -68,13 +70,28 @@ static apr_status_t read_with_timeout(ap
}
}
- if (file->pOverlapped && !file->pipe) {
+ if (file->pOverlapped && file->ftype == APR_FILETYPE_FILE) {
file->pOverlapped->Offset = (DWORD)file->filePtr;
file->pOverlapped->OffsetHigh = (DWORD)(file->filePtr >> 32);
}
- if (ReadFile(file->filehand, buf, len,
- &bytesread, file->pOverlapped)) {
+ if (file->ftype == APR_FILETYPE_SOCKET) {
+ WSABUF wsaData;
+ DWORD flags = 0;
+
+ wsaData.buf = (char*) buf;
+ wsaData.len = (u_long)len;
+ if (WSARecv((SOCKET)file->filehand, &wsaData, 1, &bytesread,
+ &flags, NULL, NULL) == SOCKET_ERROR) {
+ rv = apr_get_netos_error();
+ bytesread = 0;
+ }
+ else {
+ rv = APR_SUCCESS;
+ }
+ }
+ else if (ReadFile(file->filehand, buf, len,
+ &bytesread, file->pOverlapped)) {
rv = APR_SUCCESS;
}
else {
@@ -133,7 +150,7 @@ static apr_status_t read_with_timeout(ap
if (rv == APR_SUCCESS && bytesread == 0)
rv = APR_EOF;
- if (rv == APR_SUCCESS && file->pOverlapped && !file->pipe) {
+ if (rv == APR_SUCCESS && file->pOverlapped && file->ftype == APR_FILETYPE_FILE) {
file->filePtr += bytesread;
}
*nbytes = bytesread;
@@ -246,7 +263,7 @@ APR_DECLARE(apr_status_t) apr_file_read(
APR_DECLARE(apr_status_t) apr_file_write(apr_file_t *thefile, const void *buf, apr_size_t *nbytes)
{
apr_status_t rv;
- DWORD bwrote;
+ DWORD bwrote = 0;
/* If the file is open for xthread support, allocate and
* initialize the overlapped and io completion event (hEvent).
@@ -298,9 +315,29 @@ APR_DECLARE(apr_status_t) apr_file_write
if (thefile->flags & APR_FOPEN_XTHREAD) {
apr_thread_mutex_unlock(thefile->mutex);
}
- return rv;
- } else {
- if (!thefile->pipe) {
+ }
+ else if (thefile->ftype == APR_FILETYPE_SOCKET) {
+ WSABUF wsaData;
+ DWORD flags = 0;
+
+ wsaData.buf = (char*) buf;
+ wsaData.len = (u_long)*nbytes;
+ if (WSASend((SOCKET)file->filehand, &wsaData, 1, &bwrote,
+ flags, NULL, NULL) == SOCKET_ERROR) {
+ rv = apr_get_netos_error();
+ bwrote = 0;
+ }
+ else {
+ rv = APR_SUCCESS;
+ }
+ *nbytes = bwrote;
+ }
+ else {
+ if (thefile->ftype != APR_FILETYPE_FILE) {
+ rv = WriteFile(thefile->filehand, buf, (DWORD)*nbytes, &bwrote,
+ thefile->pOverlapped);
+ }
+ else {
apr_off_t offset = 0;
apr_status_t rc;
if (thefile->append) {
@@ -332,10 +369,6 @@ APR_DECLARE(apr_status_t) apr_file_write
apr_thread_mutex_unlock(thefile->mutex);
}
}
- else {
- rv = WriteFile(thefile->filehand, buf, (DWORD)*nbytes, &bwrote,
- thefile->pOverlapped);
- }
if (rv) {
*nbytes = bwrote;
rv = APR_SUCCESS;
@@ -382,7 +415,7 @@ APR_DECLARE(apr_status_t) apr_file_write
}
}
}
- if (rv == APR_SUCCESS && thefile->pOverlapped && !thefile->pipe) {
+ if (rv == APR_SUCCESS && thefile->pOverlapped && thefile->ftype == APR_FILETYPE_FILE) {
thefile->filePtr += *nbytes;
}
}
Modified: apr/apr/branches/1.7.x/include/arch/unix/apr_arch_poll_private.h
URL: http://svn.apache.org/viewvc/apr/apr/branches/1.7.x/include/arch/unix/apr_arch_poll_private.h?rev=1896510&r1=1896509&r2=1896510&view=diff
==============================================================================
--- apr/apr/branches/1.7.x/include/arch/unix/apr_arch_poll_private.h (original)
+++ apr/apr/branches/1.7.x/include/arch/unix/apr_arch_poll_private.h Wed Dec 29 16:53:51 2021
@@ -123,6 +123,7 @@ struct apr_pollset_t
/* Pipe descriptors used for wakeup */
apr_file_t *wakeup_pipe[2];
apr_pollfd_t wakeup_pfd;
+ volatile apr_uint32_t wakeup_set;
apr_pollset_private_t *p;
const apr_pollset_provider_t *provider;
};
@@ -151,6 +152,7 @@ struct apr_pollcb_t {
/* Pipe descriptors used for wakeup */
apr_file_t *wakeup_pipe[2];
apr_pollfd_t wakeup_pfd;
+ volatile apr_uint32_t wakeup_set;
int fd;
apr_pollcb_pset pollset;
apr_pollfd_t **copyset;
@@ -182,6 +184,6 @@ struct apr_pollcb_provider_t {
apr_status_t apr_poll_create_wakeup_pipe(apr_pool_t *pool, apr_pollfd_t *pfd,
apr_file_t **wakeup_pipe);
apr_status_t apr_poll_close_wakeup_pipe(apr_file_t **wakeup_pipe);
-void apr_poll_drain_wakeup_pipe(apr_file_t **wakeup_pipe);
+void apr_poll_drain_wakeup_pipe(volatile apr_uint32_t *wakeup_set, apr_file_t **wakeup_pipe);
#endif /* APR_ARCH_POLL_PRIVATE_H */
Modified: apr/apr/branches/1.7.x/include/arch/win32/apr_arch_file_io.h
URL: http://svn.apache.org/viewvc/apr/apr/branches/1.7.x/include/arch/win32/apr_arch_file_io.h?rev=1896510&r1=1896509&r2=1896510&view=diff
==============================================================================
--- apr/apr/branches/1.7.x/include/arch/win32/apr_arch_file_io.h (original)
+++ apr/apr/branches/1.7.x/include/arch/win32/apr_arch_file_io.h Wed Dec 29 16:53:51 2021
@@ -159,10 +159,16 @@ apr_status_t more_finfo(apr_finfo_t *fin
/* for apr_poll.c */
#define filedes filehand
+typedef enum {
+ APR_FILETYPE_FILE = 0,
+ APR_FILETYPE_PIPE,
+ APR_FILETYPE_SOCKET
+} apr_filetype_e;
+
struct apr_file_t {
apr_pool_t *pool;
HANDLE filehand;
- BOOLEAN pipe; /* Is this a pipe of a file? */
+ apr_filetype_e ftype; /* Is this a pipe, a socket or a file? */
OVERLAPPED *pOverlapped;
apr_interval_time_t timeout;
apr_int32_t flags;
Modified: apr/apr/branches/1.7.x/network_io/os2/sockopt.c
URL: http://svn.apache.org/viewvc/apr/apr/branches/1.7.x/network_io/os2/sockopt.c?rev=1896510&r1=1896509&r2=1896510&view=diff
==============================================================================
--- apr/apr/branches/1.7.x/network_io/os2/sockopt.c (original)
+++ apr/apr/branches/1.7.x/network_io/os2/sockopt.c Wed Dec 29 16:53:51 2021
@@ -32,8 +32,22 @@
APR_DECLARE(apr_status_t) apr_socket_timeout_set(apr_socket_t *sock,
apr_interval_time_t t)
{
+ apr_status_t rv = APR_SUCCESS;
+
+ /* If our new timeout is non-negative and our old timeout was
+ * negative, then we need to ensure that we are non-blocking.
+ * Conversely, if our new timeout is negative and we had
+ * non-negative timeout, we must make sure our socket is blocking.
+ */
+ if (t == 0 && sock->timeout != 0) {
+ rv = apr_socket_opt_set(sock, APR_SO_NONBLOCK, 1);
+ }
+ else if (t != 0 && sock->timeout == 0) {
+ rv = apr_socket_opt_set(sock, APR_SO_NONBLOCK, 0);
+ }
+
sock->timeout = t;
- return APR_SUCCESS;
+ return rv;
}
Modified: apr/apr/branches/1.7.x/poll/os2/pollset.c
URL: http://svn.apache.org/viewvc/apr/apr/branches/1.7.x/poll/os2/pollset.c?rev=1896510&r1=1896509&r2=1896510&view=diff
==============================================================================
--- apr/apr/branches/1.7.x/poll/os2/pollset.c (original)
+++ apr/apr/branches/1.7.x/poll/os2/pollset.c Wed Dec 29 16:53:51 2021
@@ -67,7 +67,6 @@ APR_DECLARE(apr_status_t) apr_pollset_cr
if (rc == APR_SUCCESS) {
apr_sockaddr_t *listen_address;
- apr_socket_timeout_set((*pollset)->wake_listen, 0);
apr_sockaddr_info_get(&listen_address, "", APR_UNIX, 0, 0, p);
rc = apr_socket_bind((*pollset)->wake_listen, listen_address);
@@ -80,6 +79,7 @@ APR_DECLARE(apr_status_t) apr_pollset_cr
wake_poll_fd.client_data = NULL;
apr_pollset_add(*pollset, &wake_poll_fd);
apr_socket_addr_get(&(*pollset)->wake_address, APR_LOCAL, (*pollset)->wake_listen);
+ apr_socket_timeout_set((*pollset)->wake_listen, 0);
rc = apr_socket_create(&(*pollset)->wake_sender, APR_UNIX, SOCK_DGRAM, 0, p);
}
@@ -263,17 +263,14 @@ APR_DECLARE(apr_status_t) apr_pollset_po
if (rtnevents) {
if (i == 0 && pollset->wake_listen != NULL) {
+ char ch;
+ apr_size_t len = 1;
struct apr_sockaddr_t from_addr;
- char buffer[16];
- apr_size_t buflen;
- for (;;) {
- buflen = sizeof(buffer);
- rv = apr_socket_recvfrom(&from_addr, pollset->wake_listen,
- MSG_DONTWAIT, buffer, &buflen);
- if (rv != APR_SUCCESS) {
- break;
- }
- /* Woken up, drain the pipe still. */
+ rv = apr_socket_recvfrom(&from_addr, pollset->wake_listen,
+ MSG_DONTWAIT, &ch, &len);
+ if (rv == APR_SUCCESS) {
+ /* Woken up, senders can fill the pipe again */
+ apr_atomic_set32(&pollset->wakeup_set, 0);
rc = APR_EINTR;
}
}
@@ -298,12 +295,15 @@ APR_DECLARE(apr_status_t) apr_pollset_po
APR_DECLARE(apr_status_t) apr_pollset_wakeup(apr_pollset_t *pollset)
{
- if (pollset->wake_sender) {
+ if (!pollset->wake_sender)
+ return APR_EINIT;
+
+ if (apr_atomic_cas32(&pollset->wakeup_set, 1, 0) == 0) {
apr_size_t len = 1;
return apr_socket_sendto(pollset->wake_sender, pollset->wake_address, 0, "", &len);
}
- return APR_EINIT;
+ return APR_SUCCESS;
}
Modified: apr/apr/branches/1.7.x/poll/unix/epoll.c
URL: http://svn.apache.org/viewvc/apr/apr/branches/1.7.x/poll/unix/epoll.c?rev=1896510&r1=1896509&r2=1896510&view=diff
==============================================================================
--- apr/apr/branches/1.7.x/poll/unix/epoll.c (original)
+++ apr/apr/branches/1.7.x/poll/unix/epoll.c Wed Dec 29 16:53:51 2021
@@ -289,7 +289,7 @@ static apr_status_t impl_pollset_poll(ap
if ((pollset->flags & APR_POLLSET_WAKEABLE) &&
fdptr->desc_type == APR_POLL_FILE &&
fdptr->desc.f == pollset->wakeup_pipe[0]) {
- apr_poll_drain_wakeup_pipe(pollset->wakeup_pipe);
+ apr_poll_drain_wakeup_pipe(&pollset->wakeup_set, pollset->wakeup_pipe);
rv = APR_EINTR;
}
else {
@@ -460,7 +460,7 @@ static apr_status_t impl_pollcb_poll(apr
if ((pollcb->flags & APR_POLLSET_WAKEABLE) &&
pollfd->desc_type == APR_POLL_FILE &&
pollfd->desc.f == pollcb->wakeup_pipe[0]) {
- apr_poll_drain_wakeup_pipe(pollcb->wakeup_pipe);
+ apr_poll_drain_wakeup_pipe(&pollcb->wakeup_set, pollcb->wakeup_pipe);
return APR_EINTR;
}
Modified: apr/apr/branches/1.7.x/poll/unix/kqueue.c
URL: http://svn.apache.org/viewvc/apr/apr/branches/1.7.x/poll/unix/kqueue.c?rev=1896510&r1=1896509&r2=1896510&view=diff
==============================================================================
--- apr/apr/branches/1.7.x/poll/unix/kqueue.c (original)
+++ apr/apr/branches/1.7.x/poll/unix/kqueue.c Wed Dec 29 16:53:51 2021
@@ -286,7 +286,7 @@ static apr_status_t impl_pollset_poll(ap
if ((pollset->flags & APR_POLLSET_WAKEABLE) &&
fd->desc_type == APR_POLL_FILE &&
fd->desc.f == pollset->wakeup_pipe[0]) {
- apr_poll_drain_wakeup_pipe(pollset->wakeup_pipe);
+ apr_poll_drain_wakeup_pipe(&pollset->wakeup_set, pollset->wakeup_pipe);
rv = APR_EINTR;
}
else {
@@ -473,7 +473,7 @@ static apr_status_t impl_pollcb_poll(apr
if ((pollcb->flags & APR_POLLSET_WAKEABLE) &&
pollfd->desc_type == APR_POLL_FILE &&
pollfd->desc.f == pollcb->wakeup_pipe[0]) {
- apr_poll_drain_wakeup_pipe(pollcb->wakeup_pipe);
+ apr_poll_drain_wakeup_pipe(&pollcb->wakeup_set, pollcb->wakeup_pipe);
return APR_EINTR;
}
Modified: apr/apr/branches/1.7.x/poll/unix/poll.c
URL: http://svn.apache.org/viewvc/apr/apr/branches/1.7.x/poll/unix/poll.c?rev=1896510&r1=1896509&r2=1896510&view=diff
==============================================================================
--- apr/apr/branches/1.7.x/poll/unix/poll.c (original)
+++ apr/apr/branches/1.7.x/poll/unix/poll.c Wed Dec 29 16:53:51 2021
@@ -279,7 +279,7 @@ static apr_status_t impl_pollset_poll(ap
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);
+ apr_poll_drain_wakeup_pipe(&pollset->wakeup_set, pollset->wakeup_pipe);
rv = APR_EINTR;
}
else {
@@ -431,7 +431,7 @@ static apr_status_t impl_pollcb_poll(apr
if ((pollcb->flags & APR_POLLSET_WAKEABLE) &&
pollfd->desc_type == APR_POLL_FILE &&
pollfd->desc.f == pollcb->wakeup_pipe[0]) {
- apr_poll_drain_wakeup_pipe(pollcb->wakeup_pipe);
+ apr_poll_drain_wakeup_pipe(&pollcb->wakeup_set, pollcb->wakeup_pipe);
return APR_EINTR;
}
Modified: apr/apr/branches/1.7.x/poll/unix/pollcb.c
URL: http://svn.apache.org/viewvc/apr/apr/branches/1.7.x/poll/unix/pollcb.c?rev=1896510&r1=1896509&r2=1896510&view=diff
==============================================================================
--- apr/apr/branches/1.7.x/poll/unix/pollcb.c (original)
+++ apr/apr/branches/1.7.x/poll/unix/pollcb.c Wed Dec 29 16:53:51 2021
@@ -23,6 +23,7 @@
#include "apr_poll.h"
#include "apr_time.h"
#include "apr_portable.h"
+#include "apr_atomic.h"
#include "apr_arch_file_io.h"
#include "apr_arch_networkio.h"
#include "apr_arch_poll_private.h"
@@ -134,6 +135,7 @@ APR_DECLARE(apr_status_t) apr_pollcb_cre
pollcb->flags = flags;
pollcb->pool = p;
pollcb->provider = provider;
+ pollcb->wakeup_set = 0;
rv = (*provider->create)(pollcb, size, p, flags);
if (rv == APR_ENOTIMPL) {
@@ -212,10 +214,13 @@ APR_DECLARE(apr_status_t) apr_pollcb_pol
APR_DECLARE(apr_status_t) apr_pollcb_wakeup(apr_pollcb_t *pollcb)
{
- if (pollcb->flags & APR_POLLSET_WAKEABLE)
- return apr_file_putc(1, pollcb->wakeup_pipe[1]);
- else
+ if (!(pollcb->flags & APR_POLLSET_WAKEABLE))
return APR_EINIT;
+
+ if (apr_atomic_cas32(&pollcb->wakeup_set, 1, 0) == 0)
+ return apr_file_putc(1, pollcb->wakeup_pipe[1]);
+
+ return APR_SUCCESS;
}
APR_DECLARE(const char *) apr_pollcb_method_name(apr_pollcb_t *pollcb)
Modified: apr/apr/branches/1.7.x/poll/unix/pollset.c
URL: http://svn.apache.org/viewvc/apr/apr/branches/1.7.x/poll/unix/pollset.c?rev=1896510&r1=1896509&r2=1896510&view=diff
==============================================================================
--- apr/apr/branches/1.7.x/poll/unix/pollset.c (original)
+++ apr/apr/branches/1.7.x/poll/unix/pollset.c Wed Dec 29 16:53:51 2021
@@ -23,6 +23,7 @@
#include "apr_poll.h"
#include "apr_time.h"
#include "apr_portable.h"
+#include "apr_atomic.h"
#include "apr_arch_file_io.h"
#include "apr_arch_networkio.h"
#include "apr_arch_poll_private.h"
@@ -144,6 +145,7 @@ APR_DECLARE(apr_status_t) apr_pollset_cr
pollset->pool = p;
pollset->flags = flags;
pollset->provider = provider;
+ pollset->wakeup_set = 0;
rv = (*provider->create)(pollset, size, p, flags);
if (rv == APR_ENOTIMPL) {
@@ -220,10 +222,13 @@ APR_DECLARE(apr_status_t) apr_pollset_de
APR_DECLARE(apr_status_t) apr_pollset_wakeup(apr_pollset_t *pollset)
{
- if (pollset->flags & APR_POLLSET_WAKEABLE)
- return apr_file_putc(1, pollset->wakeup_pipe[1]);
- else
+ if (!(pollset->flags & APR_POLLSET_WAKEABLE))
return APR_EINIT;
+
+ if (apr_atomic_cas32(&pollset->wakeup_set, 1, 0) == 0)
+ return apr_file_putc(1, pollset->wakeup_pipe[1]);
+
+ return APR_SUCCESS;
}
APR_DECLARE(apr_status_t) apr_pollset_add(apr_pollset_t *pollset,
Modified: apr/apr/branches/1.7.x/poll/unix/port.c
URL: http://svn.apache.org/viewvc/apr/apr/branches/1.7.x/poll/unix/port.c?rev=1896510&r1=1896509&r2=1896510&view=diff
==============================================================================
--- apr/apr/branches/1.7.x/poll/unix/port.c (original)
+++ apr/apr/branches/1.7.x/poll/unix/port.c Wed Dec 29 16:53:51 2021
@@ -411,7 +411,7 @@ static apr_status_t impl_pollset_poll(ap
if ((pollset->flags & APR_POLLSET_WAKEABLE) &&
ep->pfd.desc_type == APR_POLL_FILE &&
ep->pfd.desc.f == pollset->wakeup_pipe[0]) {
- apr_poll_drain_wakeup_pipe(pollset->wakeup_pipe);
+ apr_poll_drain_wakeup_pipe(&pollset->wakeup_set, pollset->wakeup_pipe);
rv = APR_EINTR;
}
else {
@@ -563,7 +563,7 @@ static apr_status_t impl_pollcb_poll(apr
if ((pollcb->flags & APR_POLLSET_WAKEABLE) &&
pollfd->desc_type == APR_POLL_FILE &&
pollfd->desc.f == pollcb->wakeup_pipe[0]) {
- apr_poll_drain_wakeup_pipe(pollcb->wakeup_pipe);
+ apr_poll_drain_wakeup_pipe(&pollcb->wakeup_set, pollcb->wakeup_pipe);
return APR_EINTR;
}
Modified: apr/apr/branches/1.7.x/poll/unix/select.c
URL: http://svn.apache.org/viewvc/apr/apr/branches/1.7.x/poll/unix/select.c?rev=1896510&r1=1896509&r2=1896510&view=diff
==============================================================================
--- apr/apr/branches/1.7.x/poll/unix/select.c (original)
+++ apr/apr/branches/1.7.x/poll/unix/select.c Wed Dec 29 16:53:51 2021
@@ -401,7 +401,7 @@ static apr_status_t impl_pollset_poll(ap
else {
if ((pollset->flags & APR_POLLSET_WAKEABLE) &&
pollset->p->query_set[i].desc.f == pollset->wakeup_pipe[0]) {
- apr_poll_drain_wakeup_pipe(pollset->wakeup_pipe);
+ apr_poll_drain_wakeup_pipe(&pollset->wakeup_set, pollset->wakeup_pipe);
rv = APR_EINTR;
continue;
}
Modified: apr/apr/branches/1.7.x/poll/unix/wakeup.c
URL: http://svn.apache.org/viewvc/apr/apr/branches/1.7.x/poll/unix/wakeup.c?rev=1896510&r1=1896509&r2=1896510&view=diff
==============================================================================
--- apr/apr/branches/1.7.x/poll/unix/wakeup.c (original)
+++ apr/apr/branches/1.7.x/poll/unix/wakeup.c Wed Dec 29 16:53:51 2021
@@ -18,6 +18,7 @@
#include "apr_poll.h"
#include "apr_time.h"
#include "apr_portable.h"
+#include "apr_atomic.h"
#include "apr_arch_file_io.h"
#include "apr_arch_networkio.h"
#include "apr_arch_poll_private.h"
@@ -33,7 +34,7 @@ apr_status_t apr_poll_create_wakeup_pipe
apr_status_t rv;
if ((rv = apr_file_socket_pipe_create(&wakeup_pipe[0], &wakeup_pipe[1],
- pool)) != APR_SUCCESS)
+ pool)) != APR_SUCCESS)
return rv;
pfd->reqevents = APR_POLLIN;
@@ -80,9 +81,9 @@ apr_status_t apr_poll_create_wakeup_pipe
{
apr_status_t rv;
+ /* Read end of the pipe is non-blocking */
if ((rv = apr_file_pipe_create_ex(&wakeup_pipe[0], &wakeup_pipe[1],
- APR_WRITE_BLOCK,
- pool)) != APR_SUCCESS)
+ APR_WRITE_BLOCK, pool)))
return rv;
pfd->p = pool;
@@ -135,18 +136,11 @@ apr_status_t apr_poll_close_wakeup_pipe(
/* Read and discard whatever is in the wakeup pipe.
*/
-void apr_poll_drain_wakeup_pipe(apr_file_t **wakeup_pipe)
+void apr_poll_drain_wakeup_pipe(volatile apr_uint32_t *wakeup_set, apr_file_t **wakeup_pipe)
{
- char rb[512];
- apr_size_t nr = sizeof(rb);
+ char ch;
- while (apr_file_read(wakeup_pipe[0], rb, &nr) == APR_SUCCESS) {
- /* Although we write just one byte to the other end of the pipe
- * during wakeup, multiple threads could call the wakeup.
- * So simply drain out from the input side of the pipe all
- * the data.
- */
- if (nr != sizeof(rb))
- break;
- }
+ (void)apr_file_getc(&ch, wakeup_pipe[0]);
+ apr_atomic_set32(wakeup_set, 0);
}
+
Re: svn commit: r1896510 - in /apr/apr/branches/1.7.x: ./ file_io/win32/ include/arch/unix/ include/arch/win32/ network_io/os2/ poll/os2/ poll/unix/
Posted by Eric Covener <co...@gmail.com>.
> I also have a high-level objection against backporting this change to
> APR 1.7.x: IMHO APR 1.7.x is a stable branch and I think that only
> regression fixes should be backported to the stable branch. r1896510
> is a significant change and as far as I understand it's not a
> regression fix. So I think it would be better to revert r1896510 and
> release it as part of APR 2.0 (or 1.8.x).
No comment on the scale/significance here, but IMO fixes to
non-regression defects are suitable for a patch/micro releases of APR.
Re: svn commit: r1896510 - in /apr/apr/branches/1.7.x: ./ file_io/win32/ include/arch/unix/ include/arch/win32/ network_io/os2/ poll/os2/ poll/unix/
Posted by William A Rowe Jr <wr...@rowe-clan.net>.
The answer to your question is whether a consumer who built against
apr<1.7.0 is going to blow up, whether they borrowed "private" API's
or not. If they were exported, it's effectively public.
Or, whether a consumer built against 1.7.1 would blow up against 1.7.0
- if that's true, we need to revert.
Those are the last of my obvious observations, I'll need to diff the
resulting include/... tree between 1.7.0 and 1.7.x branch to know for
sure, but thanks everyone for reviewing these questions.
Cheers,
Bill
On Tue, Feb 15, 2022 at 7:24 AM Ivan Zhakov <iv...@visualsvn.com> wrote:
>
> On Wed, 9 Feb 2022 at 13:47, Ivan Zhakov <iv...@visualsvn.com> wrote:
>>
>> On Tue, 8 Feb 2022 at 21:58, Evgeny Kotkov <ev...@visualsvn.com> wrote:
>>>
>>> Ivan Zhakov <iv...@visualsvn.com> writes:
>>>
>>> > This part is now in the following branch:
>>> > https://svn.ostyserver.net/svn/asf/apr/apr/branches/win32-pollset-wakeup-no-file-socket-emulation
>>> >
>>> > What do you think?
>>> >
>>> > It would be great if someone could take a look on the implementation from
>>> > the *nix perspective.
>>> > After that, I propose to merge the branch into trunk.
>>>
>>> In case this helps, I have tested the branch on Ubuntu x64 and it seems
>>> to compile and pass the tests.
>>>
>>>
>>
>> Thanks! Merged branch to trunk on r1897895.
>>
>> Backporting changes to 1.8.x in my TODO list.
>>
> Nominated this change to 1.8.x branch. Please review.
>
> --
> Ivan Zhakov
Re: svn commit: r1896510 - in /apr/apr/branches/1.7.x: ./ file_io/win32/ include/arch/unix/ include/arch/win32/ network_io/os2/ poll/os2/ poll/unix/
Posted by Ivan Zhakov <iv...@visualsvn.com>.
On Wed, 9 Feb 2022 at 13:47, Ivan Zhakov <iv...@visualsvn.com> wrote:
> On Tue, 8 Feb 2022 at 21:58, Evgeny Kotkov <ev...@visualsvn.com>
> wrote:
>
>> Ivan Zhakov <iv...@visualsvn.com> writes:
>>
>> > This part is now in the following branch:
>> >
>> https://svn.ostyserver.net/svn/asf/apr/apr/branches/win32-pollset-wakeup-no-file-socket-emulation
>> >
>> > What do you think?
>> >
>> > It would be great if someone could take a look on the implementation
>> from
>> > the *nix perspective.
>> > After that, I propose to merge the branch into trunk.
>>
>> In case this helps, I have tested the branch on Ubuntu x64 and it seems
>> to compile and pass the tests.
>>
>>
>>
> Thanks! Merged branch to trunk on r1897895.
>
> Backporting changes to 1.8.x in my TODO list.
>
> Nominated this change to 1.8.x branch. Please review.
--
Ivan Zhakov
Re: svn commit: r1896510 - in /apr/apr/branches/1.7.x: ./ file_io/win32/ include/arch/unix/ include/arch/win32/ network_io/os2/ poll/os2/ poll/unix/
Posted by Ivan Zhakov <iv...@visualsvn.com>.
On Tue, 8 Feb 2022 at 21:58, Evgeny Kotkov <ev...@visualsvn.com>
wrote:
> Ivan Zhakov <iv...@visualsvn.com> writes:
>
> > This part is now in the following branch:
> >
> https://svn.ostyserver.net/svn/asf/apr/apr/branches/win32-pollset-wakeup-no-file-socket-emulation
> >
> > What do you think?
> >
> > It would be great if someone could take a look on the implementation from
> > the *nix perspective.
> > After that, I propose to merge the branch into trunk.
>
> In case this helps, I have tested the branch on Ubuntu x64 and it seems
> to compile and pass the tests.
>
>
>
Thanks! Merged branch to trunk on r1897895.
Backporting changes to 1.8.x in my TODO list.
--
Ivan Zhakov
Re: svn commit: r1896510 - in /apr/apr/branches/1.7.x: ./ file_io/win32/ include/arch/unix/ include/arch/win32/ network_io/os2/ poll/os2/ poll/unix/
Posted by Evgeny Kotkov <ev...@visualsvn.com>.
Ivan Zhakov <iv...@visualsvn.com> writes:
> This part is now in the following branch:
> https://svn.ostyserver.net/svn/asf/apr/apr/branches/win32-pollset-wakeup-no-file-socket-emulation
>
> What do you think?
>
> It would be great if someone could take a look on the implementation from
> the *nix perspective.
> After that, I propose to merge the branch into trunk.
In case this helps, I have tested the branch on Ubuntu x64 and it seems
to compile and pass the tests.
(While here, I noticed that APR trunk doesn't build on Windows;
have posted on that separately.)
Thanks,
Evgeny Kotkov
Re: svn commit: r1896510 - in /apr/apr/branches/1.7.x: ./ file_io/win32/ include/arch/unix/ include/arch/win32/ network_io/os2/ poll/os2/ poll/unix/
Posted by Ivan Zhakov <iv...@visualsvn.com>.
On Thu, 20 Jan 2022 at 17:39, Ivan Zhakov <iv...@visualsvn.com> wrote:
> On Fri, 14 Jan 2022 at 18:19, Ivan Zhakov <iv...@visualsvn.com> wrote:
>
>> On Thu, 13 Jan 2022 at 23:37, Ruediger Pluem <rp...@apache.org> wrote:
>>
>>>
>>>
>>> On 1/13/22 7:04 PM, Ivan Zhakov wrote:
>>> > [[ sorry for delayed response ]]
>>> >
>>> > On Fri, 7 Jan 2022 at 17:33, Yann Ylavic <yl...@gmail.com> wrote:
>>> >>
>>> >> Hi Ivan,
>>> >>
>>> >> On Fri, Jan 7, 2022 at 2:50 PM Ivan Zhakov <iv...@visualsvn.com>
>>> wrote:
>>> >>>
>>> >>> This change does not compile on Windows in APR 1.7.x:
>>> >>> [[[
>>> >>> file_io\win32\readwrite.c(325): error C2065: 'file': undeclared
>>> identifier
>>> >>> file_io\win32\readwrite.c(325): error C2223: left of '->filehand'
>>> must
>>> >>> point to struct/union
>>> >>
>>> >> I was missing backport of r1895178, does r1896808 compile now?
>>> >> (Sorry, no Windows at hand..).
>>> > Yes, it builds now. Thanks!
>>> >
>>> >>
>>> >>>
>>> >>> I also have a high-level objection against backporting this change to
>>> >>> APR 1.7.x: IMHO APR 1.7.x is a stable branch and I think that only
>>> >>> regression fixes should be backported to the stable branch. r1896510
>>> >>> is a significant change and as far as I understand it's not a
>>> >>> regression fix. So I think it would be better to revert r1896510 and
>>> >>> release it as part of APR 2.0 (or 1.8.x).
>>> >>
>>> >> I think that most if not all of the changes to 1.7.x since 1.7.0 are
>>> >> fixes for bugs that were there before 1.7 already, not regressions
>>> >> introduced by 1.7.0.
>>> >
>>> > Agreed on the bugfix/regressions part. I have misunderstood that
>>> > r1896510 is a bugfix, perhaps, due to its size, and was thinking that
>>> > it adds new functionality. But even with that in mind, I still think
>>> > that the size of the change might be just too large for it to be an
>>> > appropriate fit for a patch release.
>>> >
>>> > Speaking of the change itself, I think that there might be an
>>> > alternative to making the apr_file_t also handle sockets on Windows.
>>> > It might be better to specifically change the pollset implementation
>>> > so that on Windows it would add a socket and use it for wakeup,
>>> > instead of using the socket disguised as a file.
>>> >
>>> > If this alternative approach sounds fine, I could try to implement it.
>>>
>>> But this could wait for a 1.7.2, correct? I am asking because there is
>>> some desire to get 1.7.1 out of the door soon.
>>> And yes I would be happy with 1.7.2 that only adds this over 1.7.1 and
>>> is released soon after 1.7.2.
>>>
>>> 1. Revert this change from 1.7.x
>> 2. Release 1.7.1
>> 3. Rework this code on trunk without changing the apr_file_t's behavior
>>
> This part is now in the following branch:
>
> https://svn.ostyserver.net/svn/asf/apr/apr/branches/win32-pollset-wakeup-no-file-socket-emulation
>
> Sorry, wrong branch URL. The correct URL is:
https://svn.apache.org/repos/asf/apr/apr/branches/win32-pollset-wakeup-no-file-socket-emulation
--
Ivan Zhakov
Re: svn commit: r1896510 - in /apr/apr/branches/1.7.x: ./ file_io/win32/ include/arch/unix/ include/arch/win32/ network_io/os2/ poll/os2/ poll/unix/
Posted by Ivan Zhakov <iv...@visualsvn.com>.
On Fri, 14 Jan 2022 at 18:19, Ivan Zhakov <iv...@visualsvn.com> wrote:
> On Thu, 13 Jan 2022 at 23:37, Ruediger Pluem <rp...@apache.org> wrote:
>
>>
>>
>> On 1/13/22 7:04 PM, Ivan Zhakov wrote:
>> > [[ sorry for delayed response ]]
>> >
>> > On Fri, 7 Jan 2022 at 17:33, Yann Ylavic <yl...@gmail.com> wrote:
>> >>
>> >> Hi Ivan,
>> >>
>> >> On Fri, Jan 7, 2022 at 2:50 PM Ivan Zhakov <iv...@visualsvn.com> wrote:
>> >>>
>> >>> This change does not compile on Windows in APR 1.7.x:
>> >>> [[[
>> >>> file_io\win32\readwrite.c(325): error C2065: 'file': undeclared
>> identifier
>> >>> file_io\win32\readwrite.c(325): error C2223: left of '->filehand' must
>> >>> point to struct/union
>> >>
>> >> I was missing backport of r1895178, does r1896808 compile now?
>> >> (Sorry, no Windows at hand..).
>> > Yes, it builds now. Thanks!
>> >
>> >>
>> >>>
>> >>> I also have a high-level objection against backporting this change to
>> >>> APR 1.7.x: IMHO APR 1.7.x is a stable branch and I think that only
>> >>> regression fixes should be backported to the stable branch. r1896510
>> >>> is a significant change and as far as I understand it's not a
>> >>> regression fix. So I think it would be better to revert r1896510 and
>> >>> release it as part of APR 2.0 (or 1.8.x).
>> >>
>> >> I think that most if not all of the changes to 1.7.x since 1.7.0 are
>> >> fixes for bugs that were there before 1.7 already, not regressions
>> >> introduced by 1.7.0.
>> >
>> > Agreed on the bugfix/regressions part. I have misunderstood that
>> > r1896510 is a bugfix, perhaps, due to its size, and was thinking that
>> > it adds new functionality. But even with that in mind, I still think
>> > that the size of the change might be just too large for it to be an
>> > appropriate fit for a patch release.
>> >
>> > Speaking of the change itself, I think that there might be an
>> > alternative to making the apr_file_t also handle sockets on Windows.
>> > It might be better to specifically change the pollset implementation
>> > so that on Windows it would add a socket and use it for wakeup,
>> > instead of using the socket disguised as a file.
>> >
>> > If this alternative approach sounds fine, I could try to implement it.
>>
>> But this could wait for a 1.7.2, correct? I am asking because there is
>> some desire to get 1.7.1 out of the door soon.
>> And yes I would be happy with 1.7.2 that only adds this over 1.7.1 and is
>> released soon after 1.7.2.
>>
>> 1. Revert this change from 1.7.x
> 2. Release 1.7.1
> 3. Rework this code on trunk without changing the apr_file_t's behavior
>
This part is now in the following branch:
https://svn.ostyserver.net/svn/asf/apr/apr/branches/win32-pollset-wakeup-no-file-socket-emulation
What do you think?
It would be great if someone could take a look on the implementation from
the *nix perspective. After that, I propose to merge the branch into trunk.
(I will be travelling/on vacation for a couple of weeks, so might not be
able to answer promptly.)
--
Ivan Zhakov
Re: svn commit: r1896510 - in /apr/apr/branches/1.7.x: ./ file_io/win32/ include/arch/unix/ include/arch/win32/ network_io/os2/ poll/os2/ poll/unix/
Posted by Yann Ylavic <yl...@gmail.com>.
On Fri, Jan 14, 2022 at 4:20 PM Ivan Zhakov <iv...@visualsvn.com> wrote:
>
> 1. Revert this change from 1.7.x
Done in r1897222.
Regards;
Yann.
Re: svn commit: r1896510 - in /apr/apr/branches/1.7.x: ./ file_io/win32/ include/arch/unix/ include/arch/win32/ network_io/os2/ poll/os2/ poll/unix/
Posted by Yann Ylavic <yl...@gmail.com>.
On Fri, Jan 14, 2022 at 4:40 PM Yann Ylavic <yl...@gmail.com> wrote:
>
> On Fri, Jan 14, 2022 at 4:20 PM Ivan Zhakov <iv...@visualsvn.com> wrote:
> >
> > 1. Revert this change from 1.7.x
> > 2. Release 1.7.1
> > 3. Rework this code on trunk without changing the apr_file_t's behavior
> > 4. Backport it to 1.7.x/1.8.x
> >
> > And if this plan makes sense, I am ready to proceed with steps (1), (3) and (4).
>
> +1, not sure we'd later release both 1.7.2 and 1.8.0 later on,
> probably only the latter.
Btw, I don't think the _behaviour_ of apr_file_t changed, the
implementation of apr_file_socket_pipe did but it should be
transparent for the user.
>
> Cheers;
> Yann.
Re: svn commit: r1896510 - in /apr/apr/branches/1.7.x: ./ file_io/win32/ include/arch/unix/ include/arch/win32/ network_io/os2/ poll/os2/ poll/unix/
Posted by Yann Ylavic <yl...@gmail.com>.
On Fri, Jan 14, 2022 at 4:20 PM Ivan Zhakov <iv...@visualsvn.com> wrote:
>
> 1. Revert this change from 1.7.x
> 2. Release 1.7.1
> 3. Rework this code on trunk without changing the apr_file_t's behavior
> 4. Backport it to 1.7.x/1.8.x
>
> And if this plan makes sense, I am ready to proceed with steps (1), (3) and (4).
+1, not sure we'd later release both 1.7.2 and 1.8.0 later on,
probably only the latter.
Cheers;
Yann.
Re: svn commit: r1896510 - in /apr/apr/branches/1.7.x: ./ file_io/win32/ include/arch/unix/ include/arch/win32/ network_io/os2/ poll/os2/ poll/unix/
Posted by Ivan Zhakov <iv...@visualsvn.com>.
On Thu, 13 Jan 2022 at 23:37, Ruediger Pluem <rp...@apache.org> wrote:
>
>
> On 1/13/22 7:04 PM, Ivan Zhakov wrote:
> > [[ sorry for delayed response ]]
> >
> > On Fri, 7 Jan 2022 at 17:33, Yann Ylavic <yl...@gmail.com> wrote:
> >>
> >> Hi Ivan,
> >>
> >> On Fri, Jan 7, 2022 at 2:50 PM Ivan Zhakov <iv...@visualsvn.com> wrote:
> >>>
> >>> This change does not compile on Windows in APR 1.7.x:
> >>> [[[
> >>> file_io\win32\readwrite.c(325): error C2065: 'file': undeclared
> identifier
> >>> file_io\win32\readwrite.c(325): error C2223: left of '->filehand' must
> >>> point to struct/union
> >>
> >> I was missing backport of r1895178, does r1896808 compile now?
> >> (Sorry, no Windows at hand..).
> > Yes, it builds now. Thanks!
> >
> >>
> >>>
> >>> I also have a high-level objection against backporting this change to
> >>> APR 1.7.x: IMHO APR 1.7.x is a stable branch and I think that only
> >>> regression fixes should be backported to the stable branch. r1896510
> >>> is a significant change and as far as I understand it's not a
> >>> regression fix. So I think it would be better to revert r1896510 and
> >>> release it as part of APR 2.0 (or 1.8.x).
> >>
> >> I think that most if not all of the changes to 1.7.x since 1.7.0 are
> >> fixes for bugs that were there before 1.7 already, not regressions
> >> introduced by 1.7.0.
> >
> > Agreed on the bugfix/regressions part. I have misunderstood that
> > r1896510 is a bugfix, perhaps, due to its size, and was thinking that
> > it adds new functionality. But even with that in mind, I still think
> > that the size of the change might be just too large for it to be an
> > appropriate fit for a patch release.
> >
> > Speaking of the change itself, I think that there might be an
> > alternative to making the apr_file_t also handle sockets on Windows.
> > It might be better to specifically change the pollset implementation
> > so that on Windows it would add a socket and use it for wakeup,
> > instead of using the socket disguised as a file.
> >
> > If this alternative approach sounds fine, I could try to implement it.
>
> But this could wait for a 1.7.2, correct? I am asking because there is
> some desire to get 1.7.1 out of the door soon.
> And yes I would be happy with 1.7.2 that only adds this over 1.7.1 and is
> released soon after 1.7.2.
>
> 1. Revert this change from 1.7.x
2. Release 1.7.1
3. Rework this code on trunk without changing the apr_file_t's behavior
4. Backport it to 1.7.x/1.8.x
And if this plan makes sense, I am ready to proceed with steps (1), (3) and
(4).
--
Ivan Zhakov
Re: svn commit: r1896510 - in /apr/apr/branches/1.7.x: ./ file_io/win32/ include/arch/unix/ include/arch/win32/ network_io/os2/ poll/os2/ poll/unix/
Posted by William A Rowe Jr <wr...@rowe-clan.net>.
On Thu, Jan 13, 2022 at 2:37 PM Ruediger Pluem <rp...@apache.org> wrote:
>
> On 1/13/22 7:04 PM, Ivan Zhakov wrote:
> >
> > On Fri, 7 Jan 2022 at 17:33, Yann Ylavic <yl...@gmail.com> wrote:
> >>
> >> On Fri, Jan 7, 2022 at 2:50 PM Ivan Zhakov <iv...@visualsvn.com> wrote:
> >>>
> >>> I also have a high-level objection against backporting this change to
> >>> APR 1.7.x: IMHO APR 1.7.x is a stable branch and I think that only
> >>> regression fixes should be backported to the stable branch. r1896510
> >>> is a significant change and as far as I understand it's not a
> >>> regression fix. So I think it would be better to revert r1896510 and
> >>> release it as part of APR 2.0 (or 1.8.x).
> >>
> >> I think that most if not all of the changes to 1.7.x since 1.7.0 are
> >> fixes for bugs that were there before 1.7 already, not regressions
> >> introduced by 1.7.0.
> >
> > Agreed on the bugfix/regressions part. I have misunderstood that
> > r1896510 is a bugfix, perhaps, due to its size, and was thinking that
> > it adds new functionality. But even with that in mind, I still think
> > that the size of the change might be just too large for it to be an
> > appropriate fit for a patch release.
> >
> > Speaking of the change itself, I think that there might be an
> > alternative to making the apr_file_t also handle sockets on Windows.
> > It might be better to specifically change the pollset implementation
> > so that on Windows it would add a socket and use it for wakeup,
> > instead of using the socket disguised as a file.
> >
> > If this alternative approach sounds fine, I could try to implement it.
>
> But this could wait for a 1.7.2, correct? I am asking because there is some desire to get 1.7.1 out of the door soon.
> And yes I would be happy with 1.7.2 that only adds this over 1.7.1 and is released soon after 1.7.2.
Although this doesn't "appear" to introduce an API change, based on the
scope it seems significant enough to merit a 1.8.0 release, following
1.7.1. I've forked a working branch 1.8.x to continue this effort without
interruption. Your call whether it should persist in 1.7.1, it seems that
apr >= 1.8 is an easier decision point for developers looking for these
enhancements.
Re: svn commit: r1896510 - in /apr/apr/branches/1.7.x: ./ file_io/win32/ include/arch/unix/ include/arch/win32/ network_io/os2/ poll/os2/ poll/unix/
Posted by Ruediger Pluem <rp...@apache.org>.
On 1/13/22 7:04 PM, Ivan Zhakov wrote:
> [[ sorry for delayed response ]]
>
> On Fri, 7 Jan 2022 at 17:33, Yann Ylavic <yl...@gmail.com> wrote:
>>
>> Hi Ivan,
>>
>> On Fri, Jan 7, 2022 at 2:50 PM Ivan Zhakov <iv...@visualsvn.com> wrote:
>>>
>>> This change does not compile on Windows in APR 1.7.x:
>>> [[[
>>> file_io\win32\readwrite.c(325): error C2065: 'file': undeclared identifier
>>> file_io\win32\readwrite.c(325): error C2223: left of '->filehand' must
>>> point to struct/union
>>
>> I was missing backport of r1895178, does r1896808 compile now?
>> (Sorry, no Windows at hand..).
> Yes, it builds now. Thanks!
>
>>
>>>
>>> I also have a high-level objection against backporting this change to
>>> APR 1.7.x: IMHO APR 1.7.x is a stable branch and I think that only
>>> regression fixes should be backported to the stable branch. r1896510
>>> is a significant change and as far as I understand it's not a
>>> regression fix. So I think it would be better to revert r1896510 and
>>> release it as part of APR 2.0 (or 1.8.x).
>>
>> I think that most if not all of the changes to 1.7.x since 1.7.0 are
>> fixes for bugs that were there before 1.7 already, not regressions
>> introduced by 1.7.0.
>
> Agreed on the bugfix/regressions part. I have misunderstood that
> r1896510 is a bugfix, perhaps, due to its size, and was thinking that
> it adds new functionality. But even with that in mind, I still think
> that the size of the change might be just too large for it to be an
> appropriate fit for a patch release.
>
> Speaking of the change itself, I think that there might be an
> alternative to making the apr_file_t also handle sockets on Windows.
> It might be better to specifically change the pollset implementation
> so that on Windows it would add a socket and use it for wakeup,
> instead of using the socket disguised as a file.
>
> If this alternative approach sounds fine, I could try to implement it.
But this could wait for a 1.7.2, correct? I am asking because there is some desire to get 1.7.1 out of the door soon.
And yes I would be happy with 1.7.2 that only adds this over 1.7.1 and is released soon after 1.7.2.
Regards
Rüdiger
Re: svn commit: r1896510 - in /apr/apr/branches/1.7.x: ./ file_io/win32/ include/arch/unix/ include/arch/win32/ network_io/os2/ poll/os2/ poll/unix/
Posted by Yann Ylavic <yl...@gmail.com>.
Hi Ivan;
On Thu, Jan 13, 2022 at 7:04 PM Ivan Zhakov <iv...@visualsvn.com> wrote:
>
> On Fri, 7 Jan 2022 at 17:33, Yann Ylavic <yl...@gmail.com> wrote:
> >
> > I was missing backport of r1895178, does r1896808 compile now?
> > (Sorry, no Windows at hand..).
> Yes, it builds now. Thanks!
Great, thanks for testing.
> >
> > I think that most if not all of the changes to 1.7.x since 1.7.0 are
> > fixes for bugs that were there before 1.7 already, not regressions
> > introduced by 1.7.0.
>
> Agreed on the bugfix/regressions part. I have misunderstood that
> r1896510 is a bugfix, perhaps, due to its size, and was thinking that
> it adds new functionality. But even with that in mind, I still think
> that the size of the change might be just too large for it to be an
> appropriate fit for a patch release.
The bugfix part is quite probable though still theoretical on the
httpd side. As you may know mpm_event makes heavy use of the wakeup
pipe with potentially plenty of worker threads waking up one listener
thread, and that should not block for obvious event scheme scalability
reasons. But there is actually no bug report about this since blocking
would not result in a deadlock and httpd users have not tested the new
implementation performance wise, yet. Theoretically it may happen..
So since mpm_event is unix(es) only, this change is mainly aimed at
fixing the unix implementation of the wakeup pipe (write a single byte
atomically/once until it's consumed), but that can't happen portably
on the APR side without adapting the Windows implementation given that
there is no synchronous/nonblocking ReadFile().
Using {Read,Write}File() on a SOCKET seems to be supported by Windows,
but if the SOCKET is set nonblocking it does not behave accordingly
(from what I could read on the subject), hence the switch to
WSA{Recv,Send}(). Note that there was already a special "pipe"
handling in Windows' apr_file_t, the "socket" one is just a third
case.
I don't find it too cumbersome personally, but if you have a better
option I'm all for it!
>
> Speaking of the change itself, I think that there might be an
> alternative to making the apr_file_t also handle sockets on Windows.
> It might be better to specifically change the pollset implementation
> so that on Windows it would add a socket and use it for wakeup,
> instead of using the socket disguised as a file.
AIUI, that's already what OS/2 does (using an UDP socket), having its
own apr_pollset_wakeup() implementation and its own draining directly
in apr_pollset_poll() instead of reusing the common
apr_poll_drain_wakeup_pipe(), which is the one performing the
apr_file_read() currently.
That's certainly an option for Windows too, and in that area it may be
interesting to look at an IOCB implementation for polling rather than
select (but that's probably another story given the different
semantics of IOCB vs epoll/kqueue..).
>
> If this alternative approach sounds fine, I could try to implement it.
Looks good to me, not sure it should block the current implementation
in 1.7.x though because AFAIK it passes the tests suite on Windows too
(which Mladen Turk made work and tried IIRC). The changes are not
trivial but quite straightforward given the previous/existing socket
as pipe implementation in apr_file_t.
But I don't want to push anything, mpm_event has worked like this for
quite some time now and it can probably wait for 1.8.x for this
supposed fix/improvement.
Regards;
Yann.
Re: svn commit: r1896510 - in /apr/apr/branches/1.7.x: ./ file_io/win32/ include/arch/unix/ include/arch/win32/ network_io/os2/ poll/os2/ poll/unix/
Posted by Yann Ylavic <yl...@gmail.com>.
On Thu, Jan 13, 2022 at 7:04 PM Ivan Zhakov <iv...@visualsvn.com> wrote:
>
> On Fri, 7 Jan 2022 at 17:33, Yann Ylavic <yl...@gmail.com> wrote:
> >
> > I think that most if not all of the changes to 1.7.x since 1.7.0 are
> > fixes for bugs that were there before 1.7 already, not regressions
> > introduced by 1.7.0.
>
> Agreed on the bugfix/regressions part. I have misunderstood that
> r1896510 is a bugfix, perhaps, due to its size, and was thinking that
> it adds new functionality.
So after reverting r1896510, the reported bug in [1] is still
addressed by r1894916, but only on unixes then.
[1] https://lists.apache.org/thread/mgosgwpqpxqrhh1q06z9okqgfqr46q24
Re: svn commit: r1896510 - in /apr/apr/branches/1.7.x: ./ file_io/win32/ include/arch/unix/ include/arch/win32/ network_io/os2/ poll/os2/ poll/unix/
Posted by Ivan Zhakov <iv...@visualsvn.com>.
[[ sorry for delayed response ]]
On Fri, 7 Jan 2022 at 17:33, Yann Ylavic <yl...@gmail.com> wrote:
>
> Hi Ivan,
>
> On Fri, Jan 7, 2022 at 2:50 PM Ivan Zhakov <iv...@visualsvn.com> wrote:
> >
> > This change does not compile on Windows in APR 1.7.x:
> > [[[
> > file_io\win32\readwrite.c(325): error C2065: 'file': undeclared identifier
> > file_io\win32\readwrite.c(325): error C2223: left of '->filehand' must
> > point to struct/union
>
> I was missing backport of r1895178, does r1896808 compile now?
> (Sorry, no Windows at hand..).
Yes, it builds now. Thanks!
>
> >
> > I also have a high-level objection against backporting this change to
> > APR 1.7.x: IMHO APR 1.7.x is a stable branch and I think that only
> > regression fixes should be backported to the stable branch. r1896510
> > is a significant change and as far as I understand it's not a
> > regression fix. So I think it would be better to revert r1896510 and
> > release it as part of APR 2.0 (or 1.8.x).
>
> I think that most if not all of the changes to 1.7.x since 1.7.0 are
> fixes for bugs that were there before 1.7 already, not regressions
> introduced by 1.7.0.
Agreed on the bugfix/regressions part. I have misunderstood that
r1896510 is a bugfix, perhaps, due to its size, and was thinking that
it adds new functionality. But even with that in mind, I still think
that the size of the change might be just too large for it to be an
appropriate fit for a patch release.
Speaking of the change itself, I think that there might be an
alternative to making the apr_file_t also handle sockets on Windows.
It might be better to specifically change the pollset implementation
so that on Windows it would add a socket and use it for wakeup,
instead of using the socket disguised as a file.
If this alternative approach sounds fine, I could try to implement it.
--
Ivan Zhakov
Re: svn commit: r1896510 - in /apr/apr/branches/1.7.x: ./ file_io/win32/ include/arch/unix/ include/arch/win32/ network_io/os2/ poll/os2/ poll/unix/
Posted by Ruediger Pluem <rp...@apache.org>.
On 1/7/22 3:32 PM, Yann Ylavic wrote:
> Hi Ivan,
>
> On Fri, Jan 7, 2022 at 2:50 PM Ivan Zhakov <iv...@visualsvn.com> wrote:
>>
>> This change does not compile on Windows in APR 1.7.x:
>> [[[
>> file_io\win32\readwrite.c(325): error C2065: 'file': undeclared identifier
>> file_io\win32\readwrite.c(325): error C2223: left of '->filehand' must
>> point to struct/union
>
> I was missing backport of r1895178, does r1896808 compile now?
> (Sorry, no Windows at hand..).
>
>>
>> I also have a high-level objection against backporting this change to
>> APR 1.7.x: IMHO APR 1.7.x is a stable branch and I think that only
>> regression fixes should be backported to the stable branch. r1896510
>> is a significant change and as far as I understand it's not a
>> regression fix. So I think it would be better to revert r1896510 and
>> release it as part of APR 2.0 (or 1.8.x).
>
> I think that most if not all of the changes to 1.7.x since 1.7.0 are
> fixes for bugs that were there before 1.7 already, not regressions
> introduced by 1.7.0.
That was also my read of the versioning rules. We only cannot add new stuff. For this we need to bump the minor version.
Regards
Rüdiger
Re: svn commit: r1896510 - in /apr/apr/branches/1.7.x: ./ file_io/win32/ include/arch/unix/ include/arch/win32/ network_io/os2/ poll/os2/ poll/unix/
Posted by Yann Ylavic <yl...@gmail.com>.
Hi Ivan,
On Fri, Jan 7, 2022 at 2:50 PM Ivan Zhakov <iv...@visualsvn.com> wrote:
>
> This change does not compile on Windows in APR 1.7.x:
> [[[
> file_io\win32\readwrite.c(325): error C2065: 'file': undeclared identifier
> file_io\win32\readwrite.c(325): error C2223: left of '->filehand' must
> point to struct/union
I was missing backport of r1895178, does r1896808 compile now?
(Sorry, no Windows at hand..).
>
> I also have a high-level objection against backporting this change to
> APR 1.7.x: IMHO APR 1.7.x is a stable branch and I think that only
> regression fixes should be backported to the stable branch. r1896510
> is a significant change and as far as I understand it's not a
> regression fix. So I think it would be better to revert r1896510 and
> release it as part of APR 2.0 (or 1.8.x).
I think that most if not all of the changes to 1.7.x since 1.7.0 are
fixes for bugs that were there before 1.7 already, not regressions
introduced by 1.7.0.
But I'm fine with switching to 1.8.x and releasing 1.8.0 if that's the
policy, not sure that 1.7.1 is needed then (and we'd need to revert
quite some changes in 1.7.x..).
Regards;
Yann.
Re: svn commit: r1896510 - in /apr/apr/branches/1.7.x: ./ file_io/win32/ include/arch/unix/ include/arch/win32/ network_io/os2/ poll/os2/ poll/unix/
Posted by Ivan Zhakov <iv...@visualsvn.com>.
On Wed, 29 Dec 2021 at 19:53, <yl...@apache.org> wrote:
>
> Author: ylavic
> Date: Wed Dec 29 16:53:51 2021
> New Revision: 1896510
>
> URL: http://svn.apache.org/viewvc?rev=1896510&view=rev
> Log:
> Merge r1895106, r1895111, r1895175, r1895181, r1895465 from trunk:
>
>
> Fix drain wakeup pipe issue when multiple threads call apr_pollset_wakeup/apr_pollcb_wakeup for the same pollset filling up drain pipe. Use atomics so that wakeup call is noop if some other thread allready done this
>
>
> Follow up to r1895106: now we want blocking reads on unix too so revert r1894914.
>
>
> Follow up to r1895106: Use less expensive atomics for wakeup.
>
> If pipe writers (wakeup) put a single byte until it's consumed by the
> reader (drain), it's enough to use an atomic cas for the writers (still) and
> an atomic (re)set for the reader (no more cas here).
>
> This requires that the reader never blocks on read though (e.g. spurious return
> from poll), so make the read side on the pipe non-blocking again/finally.
>
> Since synchronous non-blocking read is not a thing for Windows' Readfile(), add
> a ->socket flag to this arch's apr_file_t (like the existing ->pipe one) which
> file_socket_pipe_create() will set to make apr_file_read/write() handle
> non-blocking (nor overlapped) socket pipes with WSARecv/Send().
>
>
> Use enum instead multiple booleans
>
>
> Fix remaining change when apr_filetype_e was added
>
>
Hi Yann,
This change does not compile on Windows in APR 1.7.x:
[[[
file_io\win32\readwrite.c(325): error C2065: 'file': undeclared identifier
file_io\win32\readwrite.c(325): error C2223: left of '->filehand' must
point to struct/union
file_io\win32\readwrite.c(325): warning C4047: 'function': 'SOCKET'
differs in levels of indirection from 'WSABUF *'
file_io\win32\readwrite.c(325): warning C4024: 'WSASend': different
types for formal and actual parameter 1
file_io\win32\readwrite.c(325): warning C4047: 'function': 'LPWSABUF'
differs in levels of indirection from 'int'
file_io\win32\readwrite.c(325): warning C4024: 'WSASend': different
types for formal and actual parameter 2
file_io\win32\readwrite.c(325): warning C4047: 'function': 'DWORD'
differs in levels of indirection from 'DWORD *'
file_io\win32\readwrite.c(325): warning C4024: 'WSASend': different
types for formal and actual parameter 3
file_io\win32\readwrite.c(326): warning C4047: 'function': 'LPDWORD'
differs in levels of indirection from 'DWORD'
file_io\win32\readwrite.c(326): warning C4024: 'WSASend': different
types for formal and actual parameter 4
file_io\win32\readwrite.c(326): warning C4047: 'function': 'DWORD'
differs in levels of indirection from 'void *'
file_io\win32\readwrite.c(326): warning C4024: 'WSASend': different
types for formal and actual parameter 5
file_io\win32\readwrite.c(325): error C2198: 'WSASend': too few
arguments for call
]]]
I also have a high-level objection against backporting this change to
APR 1.7.x: IMHO APR 1.7.x is a stable branch and I think that only
regression fixes should be backported to the stable branch. r1896510
is a significant change and as far as I understand it's not a
regression fix. So I think it would be better to revert r1896510 and
release it as part of APR 2.0 (or 1.8.x).
--
Ivan Zhakov