From d4f121db9ba4d2a18e4e12c0629c1259a8b1c3f4 Mon Sep 17 00:00:00 2001 From: Adam Vessey Date: Tue, 6 Sep 2016 17:05:36 -0300 Subject: [PATCH 1/4] Minimize race condition. --- includes/datastream.inc | 54 ++++++++++++++++++++++++++++++++++------- 1 file changed, 45 insertions(+), 9 deletions(-) diff --git a/includes/datastream.inc b/includes/datastream.inc index 18bc5096..092909a0 100644 --- a/includes/datastream.inc +++ b/includes/datastream.inc @@ -445,17 +445,53 @@ function islandora_view_datastream_deliver_chunks(AbstractDatastream $datastream */ function islandora_view_datastream_retrieve_file_uri(AbstractDatastream $datastream) { module_load_include('inc', 'islandora', 'includes/mimetype.utils'); + module_load_include('inc', 'islandora', 'includes/utilities'); $extension = islandora_get_extension_for_mimetype($datastream->mimetype); $file_uri = 'temporary://chunk_' . $datastream->parent->id . '_' . $datastream->id . '_' . $datastream->createdDate->getTimestamp() . '.' . $extension; - if (!file_exists($file_uri)) { - $file = new stdClass(); - $file->uri = $file_uri; - $file->filename = drupal_basename($file_uri); - $file->filemime = $datastream->mimeType; - $file->status = 0; - $datastream->getContent($file_uri); - file_save($file); + touch(drupal_realpath($file_uri)); + $fp = fopen($file_uri, 'r+b'); + drupal_register_shutdown_function('fclose', $fp); + if (flock($fp, LOCK_SH)) { + if (feof($fp) && $datastream->size > 0) { + // Just opened at beginning of file, if beginning == EOF, need to grab it. + if (flock($fp, LOCK_EX)) { + // Upgrade to exclusive lock, write file. + $file = islandora_temp_file_entry($file_uri, $datastream->mimeType); + if ($file->filesize == $datastream->size) { + // Populated in another thread; downgrade lock and return. + flock($fp, LOCK_SH); + return $file_uri; + } + + try { + $datastream->getContent($file->uri); + $file = file_save($file); + if ($file->filesize != $datastream->size) { + throw new RepositoryException(t('Size of file downloaded for chunking does not match: Got @apparent bytes when expecting @actual.', array( + '@apparent' => $file->filesize, + '@actual' => $datastream->size, + ))); + } + // Downgrade to shared lock. + flock($fp, LOCK_SH); + } + catch (RepositoryException $e) { + file_delete($file); + throw $e; + } + } + else { + throw new Exception(t('Failed to acquire write lock when downloading @pid/@dsid for chunking.', array( + '@pid' => $datastream->parent->id, + '@dsid' => $datastream->id, + ))); + } + } + return $file_uri; } - return $file_uri; + throw new Exception(t('Failed to acquire shared lock when chunking @pid/@dsid.', array( + '@pid' => $datastream->parent->id, + '@dsid' => $datastream->id, + ))); } From 734c71db547f5a8346ad7b15520362e983aecb18 Mon Sep 17 00:00:00 2001 From: Adam Vessey Date: Thu, 24 Nov 2016 15:52:31 -0400 Subject: [PATCH 2/4] Unlock file before relinquishing control. --- includes/datastream.inc | 79 +++++++++++++++++++++++++---------------- 1 file changed, 49 insertions(+), 30 deletions(-) diff --git a/includes/datastream.inc b/includes/datastream.inc index 092909a0..a43e7b95 100644 --- a/includes/datastream.inc +++ b/includes/datastream.inc @@ -437,6 +437,9 @@ function islandora_view_datastream_deliver_chunks(AbstractDatastream $datastream /** * Creates/returns the file URI for the content of a datastream for chunking. * + * File locks are used to ensure the datastream is completely downloaded before + * attempting to serve up chunks from the file. + * * @param AbstractDatastream $datastream * An AbstractDatastream representing a datastream on a Fedora object. * @@ -451,44 +454,60 @@ function islandora_view_datastream_retrieve_file_uri(AbstractDatastream $datastr $file_uri = 'temporary://chunk_' . $datastream->parent->id . '_' . $datastream->id . '_' . $datastream->createdDate->getTimestamp() . '.' . $extension; touch(drupal_realpath($file_uri)); $fp = fopen($file_uri, 'r+b'); - drupal_register_shutdown_function('fclose', $fp); if (flock($fp, LOCK_SH)) { - if (feof($fp) && $datastream->size > 0) { - // Just opened at beginning of file, if beginning == EOF, need to grab it. - if (flock($fp, LOCK_EX)) { - // Upgrade to exclusive lock, write file. - $file = islandora_temp_file_entry($file_uri, $datastream->mimeType); - if ($file->filesize == $datastream->size) { - // Populated in another thread; downgrade lock and return. - flock($fp, LOCK_SH); - return $file_uri; + try { + if (feof($fp) && $datastream->size > 0) { + // Just opened at beginning of file, if beginning == EOF, need to grab + // it. + if (!flock($fp, LOCK_EX | LOCK_NB)) { + // Hypothetically, two threads could have a "shared" lock with an + // unpopulated file, so to avoid deadlock on the "exclusive" lock, + // drop the "shared" lock before blocking to obtain the "exclusive" + // lock. + flock($fp, LOCK_UN); } + if (flock($fp, LOCK_EX)) { + // Get exclusive lock, write file. + $file = islandora_temp_file_entry($file_uri, $datastream->mimeType); + if ($file->filesize == $datastream->size) { + // Populated in another thread while we were waiting for the + // "exclusive" lock; drop lock and return. + flock($fp, LOCK_UN); + fclose($fp); + return $file_uri; + } - try { - $datastream->getContent($file->uri); - $file = file_save($file); - if ($file->filesize != $datastream->size) { - throw new RepositoryException(t('Size of file downloaded for chunking does not match: Got @apparent bytes when expecting @actual.', array( - '@apparent' => $file->filesize, - '@actual' => $datastream->size, - ))); + try { + $datastream->getContent($file->uri); + $file = file_save($file); + if ($file->filesize != $datastream->size) { + throw new RepositoryException(t('Size of file downloaded for chunking does not match: Got @apparent bytes when expecting @actual.', array( + '@apparent' => $file->filesize, + '@actual' => $datastream->size, + ))); + } + } + catch (RepositoryException $e) { + file_delete($file); + throw $e; } - // Downgrade to shared lock. - flock($fp, LOCK_SH); } - catch (RepositoryException $e) { - file_delete($file); - throw $e; + else { + throw new Exception(t('Failed to acquire write lock when downloading @pid/@dsid for chunking.', array( + '@pid' => $datastream->parent->id, + '@dsid' => $datastream->id, + ))); } } - else { - throw new Exception(t('Failed to acquire write lock when downloading @pid/@dsid for chunking.', array( - '@pid' => $datastream->parent->id, - '@dsid' => $datastream->id, - ))); - } + flock($fp, LOCK_UN); + fclose($fp); + return $file_uri; + } + catch (Exception $e) { + flock($fp, LOCK_UN); + fclose($fp); + throw $e; } - return $file_uri; } throw new Exception(t('Failed to acquire shared lock when chunking @pid/@dsid.', array( '@pid' => $datastream->parent->id, From 73b658a28c56d525f6d2cf6ab6e254d383dbad84 Mon Sep 17 00:00:00 2001 From: Adam Vessey Date: Thu, 24 Nov 2016 16:55:26 -0400 Subject: [PATCH 3/4] Add to info about throwing exception to the function doc comment. --- includes/datastream.inc | 3 +++ 1 file changed, 3 insertions(+) diff --git a/includes/datastream.inc b/includes/datastream.inc index a43e7b95..a12b5972 100644 --- a/includes/datastream.inc +++ b/includes/datastream.inc @@ -440,6 +440,9 @@ function islandora_view_datastream_deliver_chunks(AbstractDatastream $datastream * File locks are used to ensure the datastream is completely downloaded before * attempting to serve up chunks from the file. * + * @throws RepositoryException|Exception + * Exceptions may be thrown if the file was unable to be reliably acquired. + * * @param AbstractDatastream $datastream * An AbstractDatastream representing a datastream on a Fedora object. * From 89d7c554178f140f19ff74240a83ce6ed927c1cb Mon Sep 17 00:00:00 2001 From: Adam Vessey Date: Thu, 24 Nov 2016 17:44:58 -0400 Subject: [PATCH 4/4] feof can only be true after a read attempt, and need to clear the stat cache. --- includes/datastream.inc | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/includes/datastream.inc b/includes/datastream.inc index a12b5972..96ead571 100644 --- a/includes/datastream.inc +++ b/includes/datastream.inc @@ -459,7 +459,8 @@ function islandora_view_datastream_retrieve_file_uri(AbstractDatastream $datastr $fp = fopen($file_uri, 'r+b'); if (flock($fp, LOCK_SH)) { try { - if (feof($fp) && $datastream->size > 0) { + fseek($fp, 0, SEEK_END); + if (ftell($fp) === 0 && $datastream->size > 0) { // Just opened at beginning of file, if beginning == EOF, need to grab // it. if (!flock($fp, LOCK_EX | LOCK_NB)) { @@ -482,6 +483,7 @@ function islandora_view_datastream_retrieve_file_uri(AbstractDatastream $datastr try { $datastream->getContent($file->uri); + clearstatcache($file->uri); $file = file_save($file); if ($file->filesize != $datastream->size) { throw new RepositoryException(t('Size of file downloaded for chunking does not match: Got @apparent bytes when expecting @actual.', array(