Re: fix for partial reads from cache
Bill Deegan <bill-cJFiu+DHMVC5azolltMz9laTQe2KTcn/@public.gmane.org> Tue, 27 Oct 2020 13:09:11 -0700
| Newsgroups | gmane.comp.programming.tools.scons.devel |
|---|---|
| Message-ID | <CAEyG4CEoCaa+B8DojmS2xftivs9g0KavFcge6FUoz=j_v2VXtg@mail.gmail.com> |
--===============6372485683313621477== Content-Type: multipart/alternative; boundary="000000000000985dca05b2ac9f29" --000000000000985dca05b2ac9f29 Content-Type: text/plain; charset="UTF-8" Any chance you can make a pull request for this? On Tue, Oct 27, 2020 at 1:02 PM Raven Kopelman <[email protected]> wrote: > Looks like attachment is not easy to access; here's plaintext patch > > diff -ru -x '*.pyc' SCons.orig/CacheDir.py > /usr/local/lib/python2.7/dist-packages/scons/SCons/CacheDir.py > --- SCons.orig/CacheDir.py 2020-10-23 16:12:44.773349285 -0700 > +++ /usr/local/lib/python2.7/dist-packages/scons/SCons/CacheDir.py > 2020-10-23 16:19:48.676570382 -0700 > @@ -56,7 +56,20 @@ > if fs.islink(cachefile): > fs.symlink(fs.readlink(cachefile), t.get_internal_path()) > else: > - env.copy_from_cache(cachefile, t.get_internal_path()) > + try: > + env.copy_from_cache(cachefile, t.get_internal_path()) > + except: > + try: > + # In case file was partially retrieved (and now > corrupt) > + # delete it to avoid poisoning commands like 'ar' that > + # read from the initial state of the file they are > writing > + # to. > + t.fs.unlink(t.get_internal_path()) > + except: > + pass > + > + raise > + > try: > os.utime(cachefile, None) > except OSError: > @@ -71,7 +84,7 @@ > cd = env.get_CacheDir() > cachedir, cachefile = cd.cachepath(t) > if t.fs.exists(cachefile): > - return "Retrieved `%s' from cache" % t.get_internal_path() > + return "Retrieving `%s' from cache" % t.get_internal_path() > return None > > CacheRetrieve = SCons.Action.Action(CacheRetrieveFunc, > CacheRetrieveString) > diff -ru -x '*.pyc' SCons.orig/Taskmaster.py > /usr/local/lib/python2.7/dist-packages/scons/SCons/Taskmaster.py > --- SCons.orig/Taskmaster.py 2020-10-23 16:12:44.785349378 -0700 > +++ /usr/local/lib/python2.7/dist-packages/scons/SCons/Taskmaster.py > 2020-10-23 15:55:41.521758762 -0700 > @@ -243,10 +243,13 @@ > cached_targets.append(t) > if len(cached_targets) < len(self.targets): > # Remove targets before building. It's possible that we > - # partially retrieved targets from the cache, leaving > - # them in read-only mode. That might cause the command > + # retrieved a subset of targets from the cache, leaving > + # them in an inconsistent state. That might cause the > command > # to fail. > # > + # Note that retrieve_from_cache() ensures no single > target can > + # be partially retrieved (file left in corrupt state). > + # > for t in cached_targets: > try: > t.fs.unlink(t.get_internal_path()) > > -- > *Raven Kopelman* | Team Lead, Senior Developer > > Safe Software Inc. > *T* 604.501.9985 x 331 | *F* 604.501.9965 > [email protected] | www.safe.com > > <http://www.safe.com/emailsignature> > > > On Tue, Oct 27, 2020 at 1:00 PM Raven Kopelman <[email protected]> > wrote: > >> Hi scons devs, >> >> Here's a fix to clean up reads from the cache that fail part way >> through. Absolutely experienced by us in production (non-local shared >> cache). This may be what my last UUID temp name fix was really trying to >> address, but I can imagine edge cases that the UUID fix could still improve. >> >> This could also have been fixed in Taskmaster.py near my comment change, >> but the reasons for cleanup seemed different enough to keep them distinct. >> >> Also cleaned up the logged message tense, as message is logged before the >> cache retrieval begins. >> >> Thanks! >> -- >> *Raven Kopelman* | Team Lead, Senior Developer >> >> Safe Software Inc. >> *T* 604.501.9985 x 331 | *F* 604.501.9965 >> [email protected] | www.safe.com >> >> <http://www.safe.com/emailsignature> >> > _______________________________________________ > Scons-dev mailing list > [email protected] > https://pairlist2.pair.net/mailman/listinfo/scons-dev > --000000000000985dca05b2ac9f29 Content-Type: text/html; charset="UTF-8" Content-Transfer-Encoding: quoted-printable <div dir=3D"ltr">Any chance you can make a pull request for this?<br></div>= <br><div class=3D"gmail_quote"><div dir=3D"ltr" class=3D"gmail_attr">On Tue= , Oct 27, 2020 at 1:02 PM Raven Kopelman <<a href=3D"mailto:raven.kopelm= [email protected]">[email protected]</a>> wrote:<br></div><blockquote cl= ass=3D"gmail_quote" style=3D"margin:0px 0px 0px 0.8ex;border-left:1px solid= rgb(204,204,204);padding-left:1ex"><div dir=3D"ltr"><div>Looks like attach= ment is not easy to access; here's plaintext patch</div><div><br></div>= <div>diff -ru -x '*.pyc' SCons.orig/CacheDir.py /usr/local/lib/pyth= on2.7/dist-packages/scons/SCons/CacheDir.py<br>--- SCons.orig/CacheDir.py 2= 020-10-23 16:12:44.773349285 -0700<br>+++ /usr/local/lib/python2.7/dist-pac= kages/scons/SCons/CacheDir.py 2020-10-23 16:19:48.676570382 -0700<br>@@ -56= ,7 +56,20 @@<br>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0if fs.islink(cachefile):<= br>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0fs.symlink(fs.readlink(c= achefile), t.get_internal_path())<br>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0else= :<br>- =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0env.copy_from_cache(cachefi= le, t.get_internal_path())<br>+ =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0tr= y:<br>+ =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0env.copy_fro= m_cache(cachefile, t.get_internal_path())<br>+ =C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0 =C2=A0except:<br>+ =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0try:<br>+ =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0# In case file was partially retrieved (and now corrupt)<br>+ =C2= =A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0# delete = it to avoid poisoning commands like 'ar' that<br>+ =C2=A0 =C2=A0 = =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0# read from the init= ial state of the file they are writing<br>+ =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0# to.<br>+ =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0t.fs.unlink(t.get_internal_pat= h())<br>+ =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0except:<br= >+ =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0pas= s<br>+<br>+ =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0raise<br= >+<br>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0try:<br>=C2=A0 =C2=A0= =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0os.utime(cachefile, None)<= br>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0except OSError:<br>@@ -7= 1,7 +84,7 @@<br>=C2=A0 =C2=A0 =C2=A0cd =3D env.get_CacheDir()<br>=C2=A0 =C2= =A0 =C2=A0cachedir, cachefile =3D cd.cachepath(t)<br>=C2=A0 =C2=A0 =C2=A0if= t.fs.exists(cachefile):<br>- =C2=A0 =C2=A0 =C2=A0 =C2=A0return "Retri= eved `%s' from cache" % t.get_internal_path()<br>+ =C2=A0 =C2=A0 = =C2=A0 =C2=A0return "Retrieving `%s' from cache" % t.get_inte= rnal_path()<br>=C2=A0 =C2=A0 =C2=A0return None<br>=C2=A0<br>=C2=A0CacheRetr= ieve =3D SCons.Action.Action(CacheRetrieveFunc, CacheRetrieveString)<br>dif= f -ru -x '*.pyc' SCons.orig/Taskmaster.py /usr/local/lib/python2.7/= dist-packages/scons/SCons/Taskmaster.py<br>--- SCons.orig/Taskmaster.py 202= 0-10-23 16:12:44.785349378 -0700<br>+++ /usr/local/lib/python2.7/dist-packa= ges/scons/SCons/Taskmaster.py 2020-10-23 15:55:41.521758762 -0700<br>@@ -24= 3,10 +243,13 @@<br>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0cached_targets.append(t)<br>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0= =C2=A0if len(cached_targets) < len(self.targets):<br>=C2=A0 =C2=A0 =C2= =A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0# Remove targets before buildi= ng. It's possible that we<br>- =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0 =C2=A0# partially retrieved targets from the cache, leaving<br>-= =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0# them in read-only= mode. That might cause the command<br>+ =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0= =C2=A0 =C2=A0 =C2=A0# retrieved a subset of targets from the cache, leavin= g<br>+ =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0# them in an = inconsistent state. That might cause the command<br>=C2=A0 =C2=A0 =C2=A0 = =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0# to fail.<br>=C2=A0 =C2=A0 =C2=A0= =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0#<br>+ =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0# Note that retrieve_from_cache() ensures no= single target can<br>+ =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0# be partially retrieved (file left in corrupt state).<br>+ =C2=A0 = =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0#<br>=C2=A0 =C2=A0 =C2=A0 = =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0for t in cached_targets:<br>=C2=A0= =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0try:<= br>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0 =C2=A0 =C2=A0t.fs.unlink(t.get_internal_path())</div><div><br></div>= <div><div><div dir=3D"ltr"><div dir=3D"ltr"><div><div dir=3D"ltr"><div><div= dir=3D"ltr"><div><div dir=3D"ltr"><div><div dir=3D"ltr"><div><div dir=3D"l= tr"><div><div dir=3D"ltr"><div><div dir=3D"ltr"><div><div dir=3D"ltr">--<br= ><b>Raven Kopelman</b> | Team Lead, Senior Developer<br><br><div dir=3D"ltr= ">Safe Software Inc.</div><b>T</b> 604.501.9985 x 331 | <b>F</b> 604.501.99= 65</div><div dir=3D"ltr"><a href=3D"mailto:[email protected]" target= =3D"_blank">[email protected]</a> | <a href=3D"http://www.safe.com" t= arget=3D"_blank">www.safe.com</a></div><div dir=3D"ltr"><div><br></div><div= ><div><a href=3D"http://www.safe.com/emailsignature" target=3D"_blank"><img= src=3D"http://cdn.safe.com/emailsignature.jpg"></a><br></div></div></div><= /div></div></div></div></div></div></div></div></div></div></div></div></di= v></div></div></div></div></div><br></div></div><br><div class=3D"gmail_quo= te"><div dir=3D"ltr" class=3D"gmail_attr">On Tue, Oct 27, 2020 at 1:00 PM R= aven Kopelman <<a href=3D"mailto:[email protected]" target=3D"_bla= nk">[email protected]</a>> wrote:<br></div><blockquote class=3D"gm= ail_quote" style=3D"margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,= 204,204);padding-left:1ex"><div dir=3D"ltr"><div>Hi scons devs,</div><div><= br></div><div>Here's a fix to clean up reads from the cache that fail p= art way through.=C2=A0 Absolutely experienced by us in production (non-loca= l shared cache).=C2=A0 This may be what my last UUID temp name fix was real= ly trying to address, but I can imagine edge cases that the UUID fix could = still improve.</div><div><br></div><div>This could also have been fixed in = Taskmaster.py near my comment change, but the reasons for cleanup seemed di= fferent enough to keep them distinct.<br></div><div><br></div><div>Also cle= aned up the logged message tense, as message is logged before the cache ret= rieval begins.</div><div><br></div><div>Thanks!<br></div><div><div><div dir= =3D"ltr"><div dir=3D"ltr"><div><div dir=3D"ltr"><div><div dir=3D"ltr"><div>= <div dir=3D"ltr"><div><div dir=3D"ltr"><div><div dir=3D"ltr"><div><div dir= =3D"ltr"><div><div dir=3D"ltr"><div><div dir=3D"ltr">--<br><b>Raven Kopelma= n</b> | Team Lead, Senior Developer<br><br><div dir=3D"ltr">Safe Software I= nc.</div><b>T</b> 604.501.9985 x 331 | <b>F</b> 604.501.9965</div><div dir= =3D"ltr"><a href=3D"mailto:[email protected]" target=3D"_blank">raven= [email protected]</a> | <a href=3D"http://www.safe.com" target=3D"_blank">= www.safe.com</a></div><div dir=3D"ltr"><div><br></div><div><div><a href=3D"= http://www.safe.com/emailsignature" target=3D"_blank"><img src=3D"http://cd= n.safe.com/emailsignature.jpg"></a><br></div></div></div></div></div></div>= </div></div></div></div></div></div></div></div></div></div></div></div></d= iv></div></div></div></div> </blockquote></div> _______________________________________________<br> Scons-dev mailing list<br> <a href=3D"mailto:[email protected]" target=3D"_blank">[email protected]= g</a><br> <a href=3D"https://pairlist2.pair.net/mailman/listinfo/scons-dev" rel=3D"no= referrer" target=3D"_blank">https://pairlist2.pair.net/mailman/listinfo/sco= ns-dev</a><br> </blockquote></div> --000000000000985dca05b2ac9f29-- --===============6372485683313621477== Content-Type: text/plain; charset="us-ascii" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit Content-Disposition: inline _______________________________________________ Scons-dev mailing list [email protected] https://pairlist2.pair.net/mailman/listinfo/scons-dev --===============6372485683313621477==--