| Message ID | CAHVN4mh0eUZZ3ngbiiN8QU7dntnCwmK6iCTNX9GJEF5NokAyeg@mail.gmail.com |
|---|---|
| State | Superseded |
| Headers | show |
On 07/12/16 06:05, Matt Oliver wrote: > Signed-off-by: Matt Oliver <protogonoi@gmail.com> > --- > libavformat/udp.c | 19 +++++++++++++++++-- > 1 file changed, 17 insertions(+), 2 deletions(-) > > diff --git a/libavformat/udp.c b/libavformat/udp.c > index f8c861d..0e4766f 100644 > --- a/libavformat/udp.c > +++ b/libavformat/udp.c > @@ -64,6 +64,14 @@ > #define HAVE_PTHREAD_CANCEL 0 > #endif > > +#if !HAVE_PTHREAD_CANCEL && (HAVE_THREADS && HAVE_WINSOCK2_H) > +/* Winsock2 recv function can be unblocked by shutting down and closing > the socket */ This seems dubious to me. Can you explain how this can work reliably on Windows? To offer some context, the reason that POSIX states that close() at the same time as any other operation is undefined is because it is impossible to avoid the following race: Thread 1: Load the file descriptor Enter the recv() call in the standard library Get preempted just before entering the system call Thread 2: Call close() on the file descriptor Finish closing, the file descriptor is now invalid and can be reused Thread 3 (could be thread 2 again, or a hidden part of the implementation): Make a new file (with open() or similar) Get given the same file descriptor, reused Thread 1: Start running again Actually make the recv() system call, which now refers to thread 3's file Data loss/crash/other badness Since there is no way to determine that a thread is actually inside the system call rather than preempted immediately before it there is no way in POSIX to avoid the race. An alternative implementation seeking to avoid this issue could perhaps supply such a function to determine the blocked state of another thread, or maybe file descriptors could be unique forever, but neither of these seems to apply in this case. Thanks, - Mark
On 7 December 2016 at 21:19, Mark Thompson <sw@jkqxz.net> wrote: > On 07/12/16 06:05, Matt Oliver wrote: > > Signed-off-by: Matt Oliver <protogonoi@gmail.com> > > --- > > libavformat/udp.c | 19 +++++++++++++++++-- > > 1 file changed, 17 insertions(+), 2 deletions(-) > > > > diff --git a/libavformat/udp.c b/libavformat/udp.c > > index f8c861d..0e4766f 100644 > > --- a/libavformat/udp.c > > +++ b/libavformat/udp.c > > @@ -64,6 +64,14 @@ > > #define HAVE_PTHREAD_CANCEL 0 > > #endif > > > > +#if !HAVE_PTHREAD_CANCEL && (HAVE_THREADS && HAVE_WINSOCK2_H) > > +/* Winsock2 recv function can be unblocked by shutting down and closing > > the socket */ > > This seems dubious to me. Can you explain how this can work reliably on > Windows? > > To offer some context, the reason that POSIX states that close() at the > same time as any other operation is undefined is because it is impossible > to avoid the following race: > > Thread 1: > Load the file descriptor > Enter the recv() call in the standard library > Get preempted just before entering the system call > Thread 2: > Call close() on the file descriptor > Finish closing, the file descriptor is now invalid and can be reused > Thread 3 (could be thread 2 again, or a hidden part of the implementation): > Make a new file (with open() or similar) > Get given the same file descriptor, reused > Thread 1: > Start running again > Actually make the recv() system call, which now refers to thread 3's file > Data loss/crash/other badness > > Since there is no way to determine that a thread is actually inside the > system call rather than preempted immediately before it there is no way in > POSIX to avoid the race. An alternative implementation seeking to avoid > this issue could perhaps supply such a function to determine the blocked > state of another thread, or maybe file descriptors could be unique forever, > but neither of these seems to apply in this case. > Well to be honest this patch is actually based on a suggestion from Hendrik in another thread so he maybe the better person to ask about that. I can however state that it is a commonly used Windows winsock programming technique as closesocket behaves differently on windows than it does on linux (the above is a very bad idea on linux). Calling closesocket on windows causes any existing recv calls in other threads to instantly return with an error code. But im not sure how it would handle the specific case set above. An alternative would be to then combine it with win32s ability to terminate threads.
On Wed, Dec 7, 2016 at 12:25 PM, Matt Oliver <protogonoi@gmail.com> wrote: > On 7 December 2016 at 21:19, Mark Thompson <sw@jkqxz.net> wrote: > >> On 07/12/16 06:05, Matt Oliver wrote: >> > Signed-off-by: Matt Oliver <protogonoi@gmail.com> >> > --- >> > libavformat/udp.c | 19 +++++++++++++++++-- >> > 1 file changed, 17 insertions(+), 2 deletions(-) >> > >> > diff --git a/libavformat/udp.c b/libavformat/udp.c >> > index f8c861d..0e4766f 100644 >> > --- a/libavformat/udp.c >> > +++ b/libavformat/udp.c >> > @@ -64,6 +64,14 @@ >> > #define HAVE_PTHREAD_CANCEL 0 >> > #endif >> > >> > +#if !HAVE_PTHREAD_CANCEL && (HAVE_THREADS && HAVE_WINSOCK2_H) >> > +/* Winsock2 recv function can be unblocked by shutting down and closing >> > the socket */ >> >> This seems dubious to me. Can you explain how this can work reliably on >> Windows? >> >> To offer some context, the reason that POSIX states that close() at the >> same time as any other operation is undefined is because it is impossible >> to avoid the following race: >> >> Thread 1: >> Load the file descriptor >> Enter the recv() call in the standard library >> Get preempted just before entering the system call >> Thread 2: >> Call close() on the file descriptor >> Finish closing, the file descriptor is now invalid and can be reused >> Thread 3 (could be thread 2 again, or a hidden part of the implementation): >> Make a new file (with open() or similar) >> Get given the same file descriptor, reused >> Thread 1: >> Start running again >> Actually make the recv() system call, which now refers to thread 3's file >> Data loss/crash/other badness >> >> Since there is no way to determine that a thread is actually inside the >> system call rather than preempted immediately before it there is no way in >> POSIX to avoid the race. An alternative implementation seeking to avoid >> this issue could perhaps supply such a function to determine the blocked >> state of another thread, or maybe file descriptors could be unique forever, >> but neither of these seems to apply in this case. >> > > Well to be honest this patch is actually based on a suggestion from Hendrik > in another thread so he maybe the better person to ask about that. I can > however state that it is a commonly used Windows winsock programming > technique as closesocket behaves differently on windows than it does on > linux (the above is a very bad idea on linux). Calling closesocket on > windows causes any existing recv calls in other threads to instantly return > with an error code. But im not sure how it would handle the specific case > set above. I'm not sure you can safely avoid that in this design when dealing with a fully generic library and no information or control whats going on in other threads. The "proper" win32 way of doing non-polling IO is using the Overlapped IO functions, which runs async and signals event objects when something happens - but that would practically require re-implementing the entire worker thread for win32. > An alternative would be to then combine it with win32s ability to terminate > threads. Terminating threads is never a good idea. - Hendrik
Le septidi 17 frimaire, an CCXXV, Hendrik Leppkes a écrit : > I'm not sure you can safely avoid that in this design when dealing > with a fully generic library and no information or control whats going > on in other threads. You are right, I had not realized there was a race condition here. What happens if shutdown() is performed but not close()? > The "proper" win32 way of doing non-polling IO is using the Overlapped > IO functions, which runs async and signals event objects when > something happens - but that would practically require re-implementing > the entire worker thread for win32. The receiving worker thread is less than 60 lines, the sending one is 100 lines because of the rate control code that can certainly be factored. All in all, that would not be that terrible, and if done cleanly can be reused later for more generic code.
On 8 December 2016 at 04:49, Nicolas George <george@nsup.org> wrote: > Le septidi 17 frimaire, an CCXXV, Hendrik Leppkes a écrit : > > I'm not sure you can safely avoid that in this design when dealing > > with a fully generic library and no information or control whats going > > on in other threads. > > You are right, I had not realized there was a race condition here. > > What happens if shutdown() is performed but not close()? According to msdn using shutdown instead should work, so ill up a new patch later today.
diff --git a/libavformat/udp.c b/libavformat/udp.c index f8c861d..0e4766f 100644 --- a/libavformat/udp.c +++ b/libavformat/udp.c @@ -64,6 +64,14 @@ #define HAVE_PTHREAD_CANCEL 0 #endif +#if !HAVE_PTHREAD_CANCEL && (HAVE_THREADS && HAVE_WINSOCK2_H) +/* Winsock2 recv function can be unblocked by shutting down and closing the socket */ +#define pthread_setcancelstate(x, y) +#define pthread_cancel +#undef HAVE_PTHREAD_CANCEL +#define HAVE_PTHREAD_CANCEL 1 +#endif + #if HAVE_PTHREAD_CANCEL #include "libavutil/thread.h"
Signed-off-by: Matt Oliver <protogonoi@gmail.com> --- libavformat/udp.c | 19 +++++++++++++++++-- 1 file changed, 17 insertions(+), 2 deletions(-) #endif @@ -1144,8 +1152,14 @@ static int udp_close(URLContext *h) if (s->thread_started) { int ret; // Cancel only read, as write has been signaled as success to the user - if (h->flags & AVIO_FLAG_READ) + if (h->flags & AVIO_FLAG_READ) { +# if !HAVE_PTHREAD_CANCEL && (HAVE_THREADS && HAVE_WINSOCK2_H) + closesocket(s->udp_fd); + s->udp_fd = INVALID_SOCKET; +# else pthread_cancel(s->circular_buffer_thread); +# endif + } ret = pthread_join(s->circular_buffer_thread, NULL); if (ret != 0) av_log(h, AV_LOG_ERROR, "pthread_join(): %s\n", strerror(ret)); @@ -1153,7 +1167,8 @@ static int udp_close(URLContext *h) pthread_cond_destroy(&s->cond); } #endif - closesocket(s->udp_fd); + if(s->udp_fd != INVALID_SOCKET) + closesocket(s->udp_fd); av_fifo_freep(&s->fifo); return 0; }