From b9db8592217d784a788fb7381f4783ab5541ef14 Mon Sep 17 00:00:00 2001 From: Joey Hess Date: Mon, 5 Jul 2021 13:02:40 -0400 Subject: [PATCH] addurl: Avoid crashing when used on beegfs. Sponsored-by: Dartmouth College's DANDI project --- CHANGELOG | 1 + Command/AddUrl.hs | 2 +- Utility/HtmlDetect.hs | 13 ++++- ...s__58___init_tests_FAIL_resource_busy.mdwn | 3 ++ ..._1241df23fa4861554cf9a5d65235c8a1._comment | 50 +++++++++++++++++++ 5 files changed, 67 insertions(+), 2 deletions(-) create mode 100644 doc/bugs/beegfs__58___init_tests_FAIL_resource_busy/comment_3_1241df23fa4861554cf9a5d65235c8a1._comment diff --git a/CHANGELOG b/CHANGELOG index 65bfcdc446..d22551c579 100644 --- a/CHANGELOG +++ b/CHANGELOG @@ -2,6 +2,7 @@ git-annex (8.20210631) UNRELEASED; urgency=medium * assistant: Avoid unncessary git repository repair in a situation where git fsck gets confused about a commit that is made while it's running. + * addurl: Avoid crashing when used on beegfs. -- Joey Hess Wed, 30 Jun 2021 17:55:10 -0400 diff --git a/Command/AddUrl.hs b/Command/AddUrl.hs index 911cb8b435..916cbdaa52 100644 --- a/Command/AddUrl.hs +++ b/Command/AddUrl.hs @@ -315,7 +315,7 @@ downloadWeb addunlockedmatcher o url urlinfo file = urlkey = addSizeUrlKey urlinfo $ Backend.URL.fromUrl url Nothing downloader f p = Url.withUrlOptions $ downloadUrl urlkey p [url] f go Nothing = return Nothing - go (Just tmp) = ifM (pure (not (rawOption o)) <&&> liftIO (isHtml <$> readFile (fromRawFilePath tmp))) + go (Just tmp) = ifM (pure (not (rawOption o)) <&&> liftIO (isHtmlFile (fromRawFilePath tmp))) ( tryyoutubedl tmp , normalfinish tmp ) diff --git a/Utility/HtmlDetect.hs b/Utility/HtmlDetect.hs index 9096cd4022..2e3c525815 100644 --- a/Utility/HtmlDetect.hs +++ b/Utility/HtmlDetect.hs @@ -1,6 +1,6 @@ {- html detection - - - Copyright 2017 Joey Hess + - Copyright 2017-2021 Joey Hess - - License: BSD-2-clause -} @@ -8,10 +8,12 @@ module Utility.HtmlDetect ( isHtml, isHtmlBs, + isHtmlFile, htmlPrefixLength, ) where import Text.HTML.TagSoup +import System.IO import Data.Char import qualified Data.ByteString.Lazy as B import qualified Data.ByteString.Lazy.Char8 as B8 @@ -44,6 +46,15 @@ isHtmlBs :: B.ByteString -> Bool -- looks for ascii strings. isHtmlBs = isHtml . B8.unpack +-- | Check if the file is html. +-- +-- It would be equivilant to use isHtml <$> readFile file, +-- but since that would not read all of the file, the handle +-- would remain open until it got garbage collected sometime later. +isHtmlFile :: FilePath -> IO Bool +isHtmlFile file = withFile file ReadMode $ \h -> + isHtmlBs <$> B.hGet h htmlPrefixLength + -- | How much of the beginning of a html document is needed to detect it. -- (conservatively) htmlPrefixLength :: Int diff --git a/doc/bugs/beegfs__58___init_tests_FAIL_resource_busy.mdwn b/doc/bugs/beegfs__58___init_tests_FAIL_resource_busy.mdwn index 5c018615f4..053f4319f2 100644 --- a/doc/bugs/beegfs__58___init_tests_FAIL_resource_busy.mdwn +++ b/doc/bugs/beegfs__58___init_tests_FAIL_resource_busy.mdwn @@ -83,3 +83,6 @@ on days ending with `y` it seems to work quite nicely. [[!meta author=yoh]] [[!tag projects/dandi]] + +> [[fixed|done]], I think, though have not installed beegfs to test. +> --[[Joey]] diff --git a/doc/bugs/beegfs__58___init_tests_FAIL_resource_busy/comment_3_1241df23fa4861554cf9a5d65235c8a1._comment b/doc/bugs/beegfs__58___init_tests_FAIL_resource_busy/comment_3_1241df23fa4861554cf9a5d65235c8a1._comment new file mode 100644 index 0000000000..17084e83a4 --- /dev/null +++ b/doc/bugs/beegfs__58___init_tests_FAIL_resource_busy/comment_3_1241df23fa4861554cf9a5d65235c8a1._comment @@ -0,0 +1,50 @@ +[[!comment format=mdwn + username="joey" + subject="""comment 3""" + date="2021-07-05T16:18:39Z" + content=""" +I've checked with strace, to see if the file was open while it was being +renamed. Not that there is anything generally wrong with renaming an open +file on a POSIX file system, but it would possibly be a problem on windows, +where some forms of opening a file locks it in place. And apparently +this filesystem is not trying to be very POSIX either. + + 413026 openat(AT_FDCWD, ".git/annex/tmp/URL-s3--file&c%%%tmp%foo", O_WRONLY|O_CREAT|O_NOCTTY|O_NONBLOCK, 0666) = 17 + 413026 write(17, "hi\n", 3) = 3 + 413026 close(17) = 0 + ... + 413026 openat(AT_FDCWD, ".git/annex/tmp/URL-s3--file&c%%%tmp%foo", O_RDONLY|O_NOCTTY|O_NONBLOCK) = 11 + 413026 read(11, "hi\n", 8192) = 3 + ... + 413026 openat(AT_FDCWD, ".git/annex/tmp/URL-s3--file&c%%%tmp%foo", O_RDONLY|O_NOCTTY|O_NONBLOCK + 413028 <... futex resumed>) = 0 + 413026 <... openat resumed>) = 16 + ... + 413026 read(16, "hi\n", 32752) = 3 + ... + 413026 close(16) = 0 + ... + 413026 rename(".git/annex/tmp/URL-s3--file&c%%%tmp%foo", "_tmp_foo") = 0 + ... + 413028 close(11) = 0 + +So the file is left open across the rename, which ought to be able to be +changed and would presumably fix the problem. + +It's also a bit odd that the file gets read twice after being copied, +once for checksum makes sense, but what's the other one? +(Copying while checksumming should be able to avoid one of the reads, +but there is an open todo tracking progress on that.) + +Aah, the other read is when it's probing if the file is html in case it ought +to be passed off to youtube-dl. That is the read that lingers for a while, +because it's done with a lazy readFile and probing if the file is html doesn't +read to the end and close it, so the file handle lingers until the GC gets +around to closing it. Of course youtube-dl won't be able to do anything with a +file url, but git-annex doesn't know that. And anyway the failure on this +filesystem would also happen when adding a http url. + +Ok, fixed it to close the handle promptly. That should fix the test suite. +It does not seem unlikely that something else will break due to this +filesystem's unusual behavior though. +"""]] -- 2.30.2