Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
276813002:
Make it harder to leak temp files on devices (Closed)
|
Created:
6 years, 7 months ago by cjhopman Modified:
6 years, 7 months ago CC:
chromium-reviews, klundberg+watch_chromium.org, bulach+watch_chromium.org, yfriedman+watch_chromium.org, ilevy-cc_chromium.org, Yaron, craigdh Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionMake it harder to leak temp files on devices
This extracts the device temp file creation deletion into a context
manager. This context manager will delete the temporary file when
leaving the with: scope.
Also changes logic for finding a temp file from a linear search from 0
to just using a random number (still checking that the file doesn't
exist). Both of these approaches could return the same file in
consecutive calls if the earlier files aren't written to (though it is
nearly impossible with the random number approach instead of essentially
guaranteed).
Removed the temp file name patterns and just added a prefix+suffix
argument to DeviceTempFile (like tempfile.NamedTemporaryFile).
BUG=371054
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=270917
Patch Set 1 #
Total comments: 7
Patch Set 2 : #
Total comments: 2
Patch Set 3 : #Patch Set 4 : Rebase #Patch Set 5 : #
Messages
Total messages: 20 (0 generated)
jbudorick: wdyt? It seemed easy for us to accidentally leak temp files on the device, maybe this will help.
I think this is a great idea. It's like tempfile for devices. A couple of thoughts (not necessarily for this CL): - I'd like to see this in it's own module, because this is something I'd like to have post-refactor. There would clearly be some cyclical dependency issues, though. - Would it be worthwhile to have this expose a more file-like interface a la tempfile? https://codereview.chromium.org/276813002/diff/1/build/android/pylib/android_... File build/android/pylib/android_commands.py (right): https://codereview.chromium.org/276813002/diff/1/build/android/pylib/android_... build/android/pylib/android_commands.py:87: def __init__(self, android_commands, prefix="temp_file", suffix=""): nit: single quotes https://codereview.chromium.org/276813002/diff/1/build/android/pylib/android_... build/android/pylib/android_commands.py:108: def Name(self): nit: unused?
btw, cyclical dependencies are fine in Python as long as the objects aren't being referenced when the module is loaded. https://codereview.chromium.org/276813002/diff/1/build/android/pylib/android_... File build/android/pylib/android_commands.py (right): https://codereview.chromium.org/276813002/diff/1/build/android/pylib/android_... build/android/pylib/android_commands.py:1183: with DeviceTempFile(self) as temp_file: I don't think we need to support pre-2.7 Python, so I believe you can combine these "with" statements into one statement with a comma.
> I'd like to see this in it's own module, because this is > something I'd like to have post-refactor. There would > clearly be some cyclical dependency issues, though. I don't know where this would belong in the post-refactor world, so it was easier to just pull it out of AndroidCommands but leave it in that file. Let me know where to put it if you want it in its own module in this change. > Would it be worthwhile to have this expose a more file-like > interface a la tempfile? I think that that would be nice (and easier for users), but it would encourage using it like a file (in particular reading/writing small chunks). And so, I think if you want that interface you need to make reading/writing in small chunks fast (probably by actually reading/writing from/to a local file that is pushed on .flush()). I didn't think it was worth it at this point to do all that. https://codereview.chromium.org/276813002/diff/1/build/android/pylib/android_... File build/android/pylib/android_commands.py (right): https://codereview.chromium.org/276813002/diff/1/build/android/pylib/android_... build/android/pylib/android_commands.py:87: def __init__(self, android_commands, prefix="temp_file", suffix=""): On 2014/05/09 15:57:03, jbudorick wrote: > nit: single quotes Done. https://codereview.chromium.org/276813002/diff/1/build/android/pylib/android_... build/android/pylib/android_commands.py:108: def Name(self): On 2014/05/09 15:57:03, jbudorick wrote: > nit: unused? Done. https://codereview.chromium.org/276813002/diff/1/build/android/pylib/android_... build/android/pylib/android_commands.py:1183: with DeviceTempFile(self) as temp_file: On 2014/05/09 16:02:40, craigdh wrote: > I don't think we need to support pre-2.7 Python, so I believe you can combine > these "with" statements into one statement with a comma. Done.
(non-binding) lgtm I'm fine with you leaving this in AndroidCommands in this CL. I'll probably move it out somewhere down the line once more of the refactor lands.
lgtm w/ optional nit https://codereview.chromium.org/276813002/diff/20001/build/android/pylib/andr... File build/android/pylib/android_commands.py (right): https://codereview.chromium.org/276813002/diff/20001/build/android/pylib/andr... build/android/pylib/android_commands.py:97: self.name = '%s/%s-%010d%s' % ( optional nit: it might not hurt to throw time().time() in here too.
https://codereview.chromium.org/276813002/diff/20001/build/android/pylib/andr... File build/android/pylib/android_commands.py (right): https://codereview.chromium.org/276813002/diff/20001/build/android/pylib/andr... build/android/pylib/android_commands.py:97: self.name = '%s/%s-%010d%s' % ( On 2014/05/09 20:18:25, craigdh wrote: > optional nit: it might not hurt to throw time().time() in here too. Done. Rounded to an int so that we don't put a '.' in the filename (so if suffix=='' the file doesn't have anything that looks like an extension). With time in there, going back to the linear scan would probably be fine (though I guess the random helps with making successive calls return different files even if the first isn't written to yet).
The CQ bit was checked by cjhopman@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/cjhopman@chromium.org/276813002/40001
FYI, CQ is re-trying this CL (attempt #1). Please consider checking whether the failures are real, and report flakes to chrome-troopers@google.com. The failing builders are: android_aosp on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/android_aosp/builds/7...) android_chromium_gn_compile_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/android_chromium_gn_c...) android_clang_dbg on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/android_clang_dbg/bui...) android_dbg on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/android_dbg/builds/18...) chromium_presubmit on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/chromium_presubmit/bu...) ios_dbg_simulator on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/ios_dbg_simulator/bui...) ios_rel_device on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/ios_rel_device/builds...) ios_rel_device_ninja on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/ios_rel_device_ninja/...) linux_chromium_chromeos_clang_dbg on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...) linux_chromium_chromeos_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...) linux_chromium_clang_dbg on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_clang_...) linux_chromium_gn_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_gn_rel...) linux_chromium_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_rel/bu...) mac_chromium_compile_dbg on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_compile_...) mac_chromium_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_rel/buil...) win_chromium_compile_dbg on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_compile_...) win_chromium_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...) win_chromium_x64_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_x64_rel/...)
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_chromium_gn_compile_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/android_chromium_gn_c...) android_dbg on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/android_dbg/builds/18...) chromium_presubmit on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/chromium_presubmit/bu...) linux_chromium_chromeos_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...) linux_chromium_gn_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_gn_rel...) linux_chromium_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_rel/bu...)
The CQ bit was checked by cjhopman@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/cjhopman@chromium.org/276813002/60001
FYI, CQ is re-trying this CL (attempt #1). Please consider checking whether the failures are real, and report flakes to chrome-troopers@google.com. The failing builders are: android_dbg_triggered_tests on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/android_dbg_triggered...)
https://codereview.chromium.org/276813002/diff/1/build/android/pylib/android_... File build/android/pylib/android_commands.py (right): https://codereview.chromium.org/276813002/diff/1/build/android/pylib/android_... build/android/pylib/android_commands.py:1183: with DeviceTempFile(self) as temp_file: On 2014/05/09 18:55:14, cjhopman wrote: > On 2014/05/09 16:02:40, craigdh wrote: > > I don't think we need to support pre-2.7 Python, so I believe you can combine > > these "with" statements into one statement with a comma. > > Done. Undone. So, making this one with statement means that it's too long to fit on one line. Then, the only way I could find to break it was to do: with (DTF(), DTF()) as ( tf, tfs): which I find less readable than just splitting it into two with statements. If you disagree, let me know and I'll change it.
The CQ bit was checked by cjhopman@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/cjhopman@chromium.org/276813002/70003
On 2014/05/16 01:15:18, cjhopman wrote: > https://codereview.chromium.org/276813002/diff/1/build/android/pylib/android_... > File build/android/pylib/android_commands.py (right): > > https://codereview.chromium.org/276813002/diff/1/build/android/pylib/android_... > build/android/pylib/android_commands.py:1183: with DeviceTempFile(self) as > temp_file: > On 2014/05/09 18:55:14, cjhopman wrote: > > On 2014/05/09 16:02:40, craigdh wrote: > > > I don't think we need to support pre-2.7 Python, so I believe you can > combine > > > these "with" statements into one statement with a comma. > > > > Done. > Undone. So, making this one with statement means that it's too long to fit on > one line. Then, the only way I could find to break it was to do: > > with (DTF(), DTF()) as ( > tf, tfs): > > which I find less readable than just splitting it into two with statements. If > you disagree, let me know and I'll change it. agreed, lgtm
Message was sent while issue was closed.
Change committed as 270917 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
