diff options
author | Jarkko Hietaniemi <jhi@iki.fi> | 2015-11-05 19:00:01 -0500 |
---|---|---|
committer | Jarkko Hietaniemi <jhi@iki.fi> | 2015-11-23 06:55:12 -0500 |
commit | d88f7f65327a827e00ba021f9c6f474e64aa97b0 (patch) | |
tree | 71f7b15abb4b4c823ee126f9876885ef51adf4da /perlio.c | |
parent | ff25d7bcdd32f227c1d1926c13e0ca78af0e3c04 (diff) | |
download | perl-d88f7f65327a827e00ba021f9c6f474e64aa97b0.tar.gz |
cleanup the mutex use of PerlIOStdio_close
Before: clang -Wthread-safety found the flow quite suspect:
one conditional mutex lock, with two conditional unlocks.
The code *looked* like the being okay logic-wise now, but
rather fragile, so let's make it clearer and more solid
by hoisting the locking earlier.
This is a fd close, this should not be a performance sensitive spot.
And it's in PerlIOStdio, so it should be pretty dead anyway.
perlio.c:3283:18: warning: mutex 'PL_perlio_mutex' is not held on every path through here [-Wthread-safety-analysis]
result = PerlSIO_fclose(stdio);
perlio.c:3299:6: warning: releasing mutex 'PL_perlio_mutex' that was not held [-Wthread-safety-analysis]
MUTEX_UNLOCK(&PL_perlio_mutex);
Diffstat (limited to 'perlio.c')
-rw-r--r-- | perlio.c | 47 |
1 files changed, 24 insertions, 23 deletions
@@ -3239,6 +3239,28 @@ PerlIOStdio_close(pTHX_ PerlIO *f) return 0; if (stdio == stdout || stdio == stderr) return PerlIO_flush(f); + } +#ifdef USE_ITHREADS + MUTEX_LOCK(&PL_perlio_mutex); + /* Right. We need a mutex here because for a brief while we + will have the situation that fd is actually closed. Hence if + a second thread were to get into this block, its dup() would + likely return our fd as its dupfd. (after all, it is closed) + Then if we get to the dup2() first, we blat the fd back + (messing up its temporary as a side effect) only for it to + then close its dupfd (== our fd) in its close(dupfd) */ + + /* There is, of course, a race condition, that any other thread + trying to input/output/whatever on this fd will be stuffed + for the duration of this little manoeuvrer. Perhaps we + should hold an IO mutex for the duration of every IO + operation if we know that invalidate doesn't work on this + platform, but that would suck, and could kill performance. + + Except that correctness trumps speed. + Advice from klortho #11912. */ +#endif + if (invalidate) { /* Tricky - must fclose(stdio) to free memory but not close(fd) Use Sarathy's trick from maint-5.6 to invalidate the fileno slot of the FILE * @@ -3247,30 +3269,9 @@ PerlIOStdio_close(pTHX_ PerlIO *f) SAVE_ERRNO; invalidate = PerlIOStdio_invalidate_fileno(aTHX_ stdio); if (!invalidate) { -#ifdef USE_ITHREADS - MUTEX_LOCK(&PL_perlio_mutex); - /* Right. We need a mutex here because for a brief while we - will have the situation that fd is actually closed. Hence if - a second thread were to get into this block, its dup() would - likely return our fd as its dupfd. (after all, it is closed) - Then if we get to the dup2() first, we blat the fd back - (messing up its temporary as a side effect) only for it to - then close its dupfd (== our fd) in its close(dupfd) */ - - /* There is, of course, a race condition, that any other thread - trying to input/output/whatever on this fd will be stuffed - for the duration of this little manoeuvrer. Perhaps we - should hold an IO mutex for the duration of every IO - operation if we know that invalidate doesn't work on this - platform, but that would suck, and could kill performance. - - Except that correctness trumps speed. - Advice from klortho #11912. */ -#endif dupfd = PerlLIO_dup(fd); #ifdef USE_ITHREADS if (dupfd < 0) { - MUTEX_UNLOCK(&PL_perlio_mutex); /* Oh cXap. This isn't going to go well. Not sure if we can recover from here, or if closing this particular FILE * is a good idea now. */ @@ -3295,10 +3296,10 @@ PerlIOStdio_close(pTHX_ PerlIO *f) if (dupfd >= 0) { PerlLIO_dup2(dupfd,fd); PerlLIO_close(dupfd); + } #ifdef USE_ITHREADS - MUTEX_UNLOCK(&PL_perlio_mutex); + MUTEX_UNLOCK(&PL_perlio_mutex); #endif - } return result; } } |