Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2838383002:
Move Minidump Uploading IO operations off the UI thread. (Closed)
DescriptionMove Minidump Uploading IO operations off the UI thread.
Before this CL we would create a CrashFileManager in
MinidumpUploaderImpl on the UI thread. CrashFileManager requires a
directory in which to store minidumps, so creating that directory, or
ensuring that it exists, requires disk operations.
In this CL we move the creation of the CrashFileManager onto the worker
thread where the actual minidump uploading happens.
Also change the rescheduling logic for minidump uploading slightly,
Previously we would check whether there are any minidumps left, at the
time when the current job is cancelled, from the UI thread. But that
requires a disk operation, so now we instead update whether to
reschedule uploads, from the worker thread, for the UI thread to read in
a thread-safe way (without disk reads).
Note that this change affects the minidump uploading of both WebView and
Chrome.
BUG=714138
Review-Url: https://codereview.chromium.org/2838383002
Cr-Commit-Position: refs/heads/master@{#468955}
Committed: https://chromium.googlesource.com/chromium/src/+/ee63feacd7199214bf9673efd957afabe9be13bb
Patch Set 1 #
Total comments: 25
Patch Set 2 : Cancel uploads if no crash dir exists + fix mUploadsDone logic + add test and comments. #
Total comments: 14
Patch Set 3 : Remove mUploadsDone + minor test-fixes. #Patch Set 4 : Remove unused variable. #Patch Set 5 : Add logging to debug failing test. #Patch Set 6 : Don't fail test if uploadFinished called - we don't guarantee ng. #
Messages
Total messages: 25 (7 generated)
gsennton@chromium.org changed reviewers: + isherman@chromium.org, tobiasjs@chromium.org
Ilya: minidump_uploader Toby: WebView ptal :)
https://codereview.chromium.org/2838383002/diff/1/components/minidump_uploade... File components/minidump_uploader/android/java/src/org/chromium/components/minidump_uploader/MinidumpUploaderImpl.java (right): https://codereview.chromium.org/2838383002/diff/1/components/minidump_uploade... components/minidump_uploader/android/java/src/org/chromium/components/minidump_uploader/MinidumpUploaderImpl.java:159: if (mCancelUpload) return; You're leaving mUploadsDone in a potentially inconsistent state here.
https://codereview.chromium.org/2838383002/diff/1/components/minidump_uploade... File components/minidump_uploader/android/java/src/org/chromium/components/minidump_uploader/MinidumpUploaderImpl.java (right): https://codereview.chromium.org/2838383002/diff/1/components/minidump_uploade... components/minidump_uploader/android/java/src/org/chromium/components/minidump_uploader/MinidumpUploaderImpl.java:159: if (mCancelUpload) return; On 2017/04/26 13:34:30, Tobias Sargeant wrote: > You're leaving mUploadsDone in a potentially inconsistent state here. Thinking about it a bit more, I suspect this is unavoidable without doing I/O operations or blocking on the UI thread. We just have to accept, then, that we may reschedule a job that will early out because it has no work to do.
https://codereview.chromium.org/2838383002/diff/1/components/minidump_uploade... File components/minidump_uploader/android/java/src/org/chromium/components/minidump_uploader/MinidumpUploaderImpl.java (right): https://codereview.chromium.org/2838383002/diff/1/components/minidump_uploade... components/minidump_uploader/android/java/src/org/chromium/components/minidump_uploader/MinidumpUploaderImpl.java:159: if (mCancelUpload) return; On 2017/04/26 13:41:19, Tobias Sargeant wrote: > On 2017/04/26 13:34:30, Tobias Sargeant wrote: > > You're leaving mUploadsDone in a potentially inconsistent state here. > Thinking about it a bit more, I suspect this is unavoidable without doing I/O > operations or blocking on the UI thread. We just have to accept, then, that we > may reschedule a job that will early out because it has no work to do. Yeah, that's the conclusion I reached as well. I don't think this rescheduling is a major issue anyway - the only reason getUploadsDone() would return false even when there are no minidumps left is the case where we fail to upload some minidump, it reaches its maximum number of retries, and we cancel the upload job before it calls setUploadsDone.. (if we care a lot about this case we could call setUploadsDone after each minidump-upload to minimise the window during which we might incorrectly schedule a new job).
Thanks, Gustav! It would be /great/ to add test coverage for the asynchronous behavior. Not sure how approachable that is, though... https://codereview.chromium.org/2838383002/diff/1/components/minidump_uploade... File components/minidump_uploader/android/java/src/org/chromium/components/minidump_uploader/MinidumpUploaderImpl.java (right): https://codereview.chromium.org/2838383002/diff/1/components/minidump_uploade... components/minidump_uploader/android/java/src/org/chromium/components/minidump_uploader/MinidumpUploaderImpl.java:47: * Represents whether the current batch of minidump uploads is done. Let's document why this variable exists, i.e. that we want all file operations to happen off the UI thread. https://codereview.chromium.org/2838383002/diff/1/components/minidump_uploade... components/minidump_uploader/android/java/src/org/chromium/components/minidump_uploader/MinidumpUploaderImpl.java:49: private boolean mUploadsDone = false; For the same reason that mCancelUpload doesn't need a lock, does this variable need a lock? (Note: This is not a rhetorical question -- I'm definitely rusty on the finer points of synchronization!) https://codereview.chromium.org/2838383002/diff/1/components/minidump_uploade... components/minidump_uploader/android/java/src/org/chromium/components/minidump_uploader/MinidumpUploaderImpl.java:93: mUploadsDone = uploadsDone; This variable can only move in one direction, from false to true -- right? I think this state machine would be clearer if setUploadsDone did not take any params, and uniformly set the var to true here. https://codereview.chromium.org/2838383002/diff/1/components/minidump_uploade... components/minidump_uploader/android/java/src/org/chromium/components/minidump_uploader/MinidumpUploaderImpl.java:122: Log.e(TAG, "Crash directory doesn't exist!"); Should there be a return stmt here? Should we be calling setUploadsDone()? Also, mostly unrelated: Why do we want to create a crash directory from this location in the code, if it doesn't already exist? https://codereview.chromium.org/2838383002/diff/1/components/minidump_uploade... components/minidump_uploader/android/java/src/org/chromium/components/minidump_uploader/MinidumpUploaderImpl.java:126: setUploadsDone(minidumps.length == 0); This call seems unnecessary. If we're done, then the for loop won't run (there will be zero items), so we'll promptly reach the call on line 180. Well, I suppose it *could* matter if cleaning out the old files takes too long. If that's the concern, then let's document that here. https://codereview.chromium.org/2838383002/diff/1/components/minidump_uploade... components/minidump_uploader/android/java/src/org/chromium/components/minidump_uploader/MinidumpUploaderImpl.java:159: if (mCancelUpload) return; On 2017/04/26 13:58:36, gsennton wrote: > On 2017/04/26 13:41:19, Tobias Sargeant wrote: > > On 2017/04/26 13:34:30, Tobias Sargeant wrote: > > > You're leaving mUploadsDone in a potentially inconsistent state here. > > Thinking about it a bit more, I suspect this is unavoidable without doing I/O > > operations or blocking on the UI thread. We just have to accept, then, that we > > may reschedule a job that will early out because it has no work to do. > > Yeah, that's the conclusion I reached as well. > I don't think this rescheduling is a major issue anyway - the only reason > getUploadsDone() would return false even when there are no minidumps left is the > case where we fail to upload some minidump, it reaches its maximum number of > retries, and we cancel the upload job before it calls setUploadsDone.. (if we > care a lot about this case we could call setUploadsDone after each > minidump-upload to minimise the window during which we might incorrectly > schedule a new job). It's probably worth documenting this somewhere =)
https://codereview.chromium.org/2838383002/diff/1/components/minidump_uploade... File components/minidump_uploader/android/java/src/org/chromium/components/minidump_uploader/MinidumpUploaderImpl.java (right): https://codereview.chromium.org/2838383002/diff/1/components/minidump_uploade... components/minidump_uploader/android/java/src/org/chromium/components/minidump_uploader/MinidumpUploaderImpl.java:49: private boolean mUploadsDone = false; On 2017/04/27 00:57:06, Ilya Sherman wrote: > For the same reason that mCancelUpload doesn't need a lock, does this variable > need a lock? (Note: This is not a rhetorical question -- I'm definitely rusty > on the finer points of synchronization!) All that we really need to ensure is that when this variable is false, there aren't any minidumps to upload. (we already have to tolerate false positives). Making it volatile would be enough to ensure that reads aren't optimized away. https://codereview.chromium.org/2838383002/diff/1/components/minidump_uploade... components/minidump_uploader/android/java/src/org/chromium/components/minidump_uploader/MinidumpUploaderImpl.java:93: mUploadsDone = uploadsDone; On 2017/04/27 00:57:06, Ilya Sherman wrote: > This variable can only move in one direction, from false to true -- right? I > think this state machine would be clearer if setUploadsDone did not take any > params, and uniformly set the var to true here. It can move from true to false at the end of the UploadRunnable processing.
https://codereview.chromium.org/2838383002/diff/1/components/minidump_uploade... File components/minidump_uploader/android/java/src/org/chromium/components/minidump_uploader/MinidumpUploaderImpl.java (right): https://codereview.chromium.org/2838383002/diff/1/components/minidump_uploade... components/minidump_uploader/android/java/src/org/chromium/components/minidump_uploader/MinidumpUploaderImpl.java:93: mUploadsDone = uploadsDone; On 2017/04/27 12:34:49, Tobias Sargeant wrote: > On 2017/04/27 00:57:06, Ilya Sherman wrote: > > This variable can only move in one direction, from false to true -- right? I > > think this state machine would be clearer if setUploadsDone did not take any > > params, and uniformly set the var to true here. > > It can move from true to false at the end of the UploadRunnable processing. It can? Why? That suggests to me that we'd be setting it multiple times while the Runnable is executing, which seems unnecessary.
https://codereview.chromium.org/2838383002/diff/1/components/minidump_uploade... File components/minidump_uploader/android/java/src/org/chromium/components/minidump_uploader/MinidumpUploaderImpl.java (right): https://codereview.chromium.org/2838383002/diff/1/components/minidump_uploade... components/minidump_uploader/android/java/src/org/chromium/components/minidump_uploader/MinidumpUploaderImpl.java:93: mUploadsDone = uploadsDone; On 2017/04/27 22:17:30, Ilya Sherman wrote: > On 2017/04/27 12:34:49, Tobias Sargeant wrote: > > On 2017/04/27 00:57:06, Ilya Sherman wrote: > > > This variable can only move in one direction, from false to true -- right? > I > > > think this state machine would be clearer if setUploadsDone did not take any > > > params, and uniformly set the var to true here. > > > > It can move from true to false at the end of the UploadRunnable processing. > > It can? Why? That suggests to me that we'd be setting it multiple times while > the Runnable is executing, which seems unnecessary. Sorry, had the wrong sense to the boolean. https://codereview.chromium.org/2838383002/diff/1/components/minidump_uploade... components/minidump_uploader/android/java/src/org/chromium/components/minidump_uploader/MinidumpUploaderImpl.java:126: setUploadsDone(minidumps.length == 0); On 2017/04/27 00:57:06, Ilya Sherman wrote: > This call seems unnecessary. If we're done, then the for loop won't run (there > will be zero items), so we'll promptly reach the call on line 180. Well, I > suppose it *could* matter if cleaning out the old files takes too long. If > that's the concern, then let's document that here. But it (currently) needs to be set to false here if there are minidumps to upload, so that if the job is cancelled it's left at that value, and we can reschedule? Maybe the sense of the boolean should be inverted, so that it's mMinidumpsToUpload instead of mUploadsDone. Actually, that's another problem: if the system cancels the job before we get to this point, then the job won't be rescheduled. I think it's ok to set the boolean to the right sense (work to do) at the time we schedule the job, and then only reset it once we know we've finished.
Thanks for quick reviews! Hopefully, this version should be closer to final ;) https://codereview.chromium.org/2838383002/diff/1/components/minidump_uploade... File components/minidump_uploader/android/java/src/org/chromium/components/minidump_uploader/MinidumpUploaderImpl.java (right): https://codereview.chromium.org/2838383002/diff/1/components/minidump_uploade... components/minidump_uploader/android/java/src/org/chromium/components/minidump_uploader/MinidumpUploaderImpl.java:47: * Represents whether the current batch of minidump uploads is done. On 2017/04/27 00:57:07, Ilya Sherman wrote: > Let's document why this variable exists, i.e. that we want all file operations > to happen off the UI thread. Done. https://codereview.chromium.org/2838383002/diff/1/components/minidump_uploade... components/minidump_uploader/android/java/src/org/chromium/components/minidump_uploader/MinidumpUploaderImpl.java:49: private boolean mUploadsDone = false; On 2017/04/27 12:34:49, Tobias Sargeant wrote: > On 2017/04/27 00:57:06, Ilya Sherman wrote: > > For the same reason that mCancelUpload doesn't need a lock, does this variable > > need a lock? (Note: This is not a rhetorical question -- I'm definitely rusty > > on the finer points of synchronization!) > > All that we really need to ensure is that when this variable is false, there > aren't any minidumps to upload. (we already have to tolerate false positives). > > Making it volatile would be enough to ensure that reads aren't optimized away. Done. https://codereview.chromium.org/2838383002/diff/1/components/minidump_uploade... components/minidump_uploader/android/java/src/org/chromium/components/minidump_uploader/MinidumpUploaderImpl.java:93: mUploadsDone = uploadsDone; On 2017/04/28 10:50:17, Tobias Sargeant wrote: > On 2017/04/27 22:17:30, Ilya Sherman wrote: > > On 2017/04/27 12:34:49, Tobias Sargeant wrote: > > > On 2017/04/27 00:57:06, Ilya Sherman wrote: > > > > This variable can only move in one direction, from false to true -- right? > > > I > > > > think this state machine would be clearer if setUploadsDone did not take > any > > > > params, and uniformly set the var to true here. > > > > > > It can move from true to false at the end of the UploadRunnable processing. > > > > It can? Why? That suggests to me that we'd be setting it multiple times > while > > the Runnable is executing, which seems unnecessary. > > Sorry, had the wrong sense to the boolean. Done. https://codereview.chromium.org/2838383002/diff/1/components/minidump_uploade... components/minidump_uploader/android/java/src/org/chromium/components/minidump_uploader/MinidumpUploaderImpl.java:122: Log.e(TAG, "Crash directory doesn't exist!"); On 2017/04/27 00:57:06, Ilya Sherman wrote: > Should there be a return stmt here? Should we be calling setUploadsDone()? > Also, mostly unrelated: Why do we want to create a crash directory from this > location in the code, if it doesn't already exist? Good point - I already added the crashParentDir.isDirectory check (and early-out) above because it doesn't make sense to continue if the directory is empty. You are correct - if there is no directory we should just early-out. https://codereview.chromium.org/2838383002/diff/1/components/minidump_uploade... components/minidump_uploader/android/java/src/org/chromium/components/minidump_uploader/MinidumpUploaderImpl.java:126: setUploadsDone(minidumps.length == 0); On 2017/04/28 10:50:17, Tobias Sargeant wrote: > On 2017/04/27 00:57:06, Ilya Sherman wrote: > > This call seems unnecessary. If we're done, then the for loop won't run > (there > > will be zero items), so we'll promptly reach the call on line 180. Well, I > > suppose it *could* matter if cleaning out the old files takes too long. If > > that's the concern, then let's document that here. > > But it (currently) needs to be set to false here if there are minidumps to > upload, so that if the job is cancelled it's left at that value, and we can > reschedule? Maybe the sense of the boolean should be inverted, so that it's > mMinidumpsToUpload instead of mUploadsDone. > > Actually, that's another problem: if the system cancels the job before we get to > this point, then the job won't be rescheduled. I think it's ok to set the > boolean to the right sense (work to do) at the time we schedule the job, and > then only reset it once we know we've finished. We are already setting it to false (mUploadsDone == false means we should reschedule) as default (i.e. when MinidumpUploaderImpl is created), so in the default case we choose to reschedule. Or am I missing something? (we don't re-use MinidumpUploaderImpl for later uploads). https://codereview.chromium.org/2838383002/diff/1/components/minidump_uploade... components/minidump_uploader/android/java/src/org/chromium/components/minidump_uploader/MinidumpUploaderImpl.java:159: if (mCancelUpload) return; On 2017/04/27 00:57:06, Ilya Sherman wrote: > On 2017/04/26 13:58:36, gsennton wrote: > > On 2017/04/26 13:41:19, Tobias Sargeant wrote: > > > On 2017/04/26 13:34:30, Tobias Sargeant wrote: > > > > You're leaving mUploadsDone in a potentially inconsistent state here. > > > Thinking about it a bit more, I suspect this is unavoidable without doing > I/O > > > operations or blocking on the UI thread. We just have to accept, then, that > we > > > may reschedule a job that will early out because it has no work to do. > > > > Yeah, that's the conclusion I reached as well. > > I don't think this rescheduling is a major issue anyway - the only reason > > getUploadsDone() would return false even when there are no minidumps left is > the > > case where we fail to upload some minidump, it reaches its maximum number of > > retries, and we cancel the upload job before it calls setUploadsDone.. (if we > > care a lot about this case we could call setUploadsDone after each > > minidump-upload to minimise the window during which we might incorrectly > > schedule a new job). > > It's probably worth documenting this somewhere =) Done (I added a comment inside cancelUploads()). Related to this - it feels like we are adding too many comments here, not that comments in themselves are bad - it just feels like the need of so many comments says something about the complexity of the code :/ https://codereview.chromium.org/2838383002/diff/1/components/minidump_uploade... components/minidump_uploader/android/java/src/org/chromium/components/minidump_uploader/MinidumpUploaderImpl.java:221: return getUploadsDone(); Maybe this was what was confusing about the uploadsDone variable? This should be return !getUploadsDone(); :) https://codereview.chromium.org/2838383002/diff/20001/android_webview/javates... File android_webview/javatests/src/org/chromium/android_webview/test/crash/MinidumpUploaderTest.java (right): https://codereview.chromium.org/2838383002/diff/20001/android_webview/javates... android_webview/javatests/src/org/chromium/android_webview/test/crash/MinidumpUploaderTest.java:335: public void testCancelFailedUploadCausesReschedule() throws IOException { This test code is very similar (some duplication) to the tests above - but it tests one specific thing rather than a composition of several things. I.e. this entire test is all for the last line of method - assertTrue(cancelUploads()). In the spirit of the doc that Ilya shared about testing at some point I think it makes sense to duplicate the code here and just test one single thing, instead of adding the assert to a previous test (but I might also be misunderstanding the point here, so if you have any thoughts please do tell - I like adding lots of tests but I often end up with a bit of a mess, which means spending more time on test refactoring whenever the real code changes, so any feedback in this area would be great). WDYT? https://codereview.chromium.org/2838383002/diff/20001/android_webview/javates... android_webview/javatests/src/org/chromium/android_webview/test/crash/MinidumpUploaderTest.java:367: assertTrue(minidumpUploader.cancelUploads()); Note that I only assert that we reschedule uploads if the uploads fail - with our current code we will actually reschedule even if the upload happens to succeed here, but that doesn't seem like a scenario we want to enforce through testing ;).
https://codereview.chromium.org/2838383002/diff/1/components/minidump_uploade... File components/minidump_uploader/android/java/src/org/chromium/components/minidump_uploader/MinidumpUploaderImpl.java (right): https://codereview.chromium.org/2838383002/diff/1/components/minidump_uploade... components/minidump_uploader/android/java/src/org/chromium/components/minidump_uploader/MinidumpUploaderImpl.java:126: setUploadsDone(minidumps.length == 0); On 2017/04/28 13:01:20, gsennton wrote: > On 2017/04/28 10:50:17, Tobias Sargeant wrote: > > On 2017/04/27 00:57:06, Ilya Sherman wrote: > > > This call seems unnecessary. If we're done, then the for loop won't run > > (there > > > will be zero items), so we'll promptly reach the call on line 180. Well, I > > > suppose it *could* matter if cleaning out the old files takes too long. If > > > that's the concern, then let's document that here. > > > > But it (currently) needs to be set to false here if there are minidumps to > > upload, so that if the job is cancelled it's left at that value, and we can > > reschedule? Maybe the sense of the boolean should be inverted, so that it's > > mMinidumpsToUpload instead of mUploadsDone. > > > > Actually, that's another problem: if the system cancels the job before we get > to > > this point, then the job won't be rescheduled. I think it's ok to set the > > boolean to the right sense (work to do) at the time we schedule the job, and > > then only reset it once we know we've finished. > > We are already setting it to false (mUploadsDone == false means we should > reschedule) as default (i.e. when MinidumpUploaderImpl is created), so in the > default case we choose to reschedule. > Or am I missing something? (we don't re-use MinidumpUploaderImpl for later > uploads). Then Ilya is right, we only go from false to true at the end of this function, and this call can be removed. Arguably, though, the logic can then be entirely removed, because the only time that you would cancel and not reschedule is if the cancellation signal came after you left the loop. I think we could pay the extra cost of just rescheduling in that case, and remove mUploadsDone entirely.
Thanks! https://codereview.chromium.org/2838383002/diff/1/components/minidump_uploade... File components/minidump_uploader/android/java/src/org/chromium/components/minidump_uploader/MinidumpUploaderImpl.java (right): https://codereview.chromium.org/2838383002/diff/1/components/minidump_uploade... components/minidump_uploader/android/java/src/org/chromium/components/minidump_uploader/MinidumpUploaderImpl.java:126: setUploadsDone(minidumps.length == 0); On 2017/04/28 13:15:47, Tobias Sargeant wrote: > On 2017/04/28 13:01:20, gsennton wrote: > > On 2017/04/28 10:50:17, Tobias Sargeant wrote: > > > On 2017/04/27 00:57:06, Ilya Sherman wrote: > > > > This call seems unnecessary. If we're done, then the for loop won't run > > > (there > > > > will be zero items), so we'll promptly reach the call on line 180. Well, > I > > > > suppose it *could* matter if cleaning out the old files takes too long. > If > > > > that's the concern, then let's document that here. > > > > > > But it (currently) needs to be set to false here if there are minidumps to > > > upload, so that if the job is cancelled it's left at that value, and we can > > > reschedule? Maybe the sense of the boolean should be inverted, so that it's > > > mMinidumpsToUpload instead of mUploadsDone. > > > > > > Actually, that's another problem: if the system cancels the job before we > get > > to > > > this point, then the job won't be rescheduled. I think it's ok to set the > > > boolean to the right sense (work to do) at the time we schedule the job, and > > > then only reset it once we know we've finished. > > > > We are already setting it to false (mUploadsDone == false means we should > > reschedule) as default (i.e. when MinidumpUploaderImpl is created), so in the > > default case we choose to reschedule. > > Or am I missing something? (we don't re-use MinidumpUploaderImpl for later > > uploads). > > Then Ilya is right, we only go from false to true at the end of this function, > and this call can be removed. > > Arguably, though, the logic can then be entirely removed, because the only time > that you would cancel and not reschedule is if the cancellation signal came > after you left the loop. I think we could pay the extra cost of just > rescheduling in that case, and remove mUploadsDone entirely. Yeah, actually, I think that's even better. https://codereview.chromium.org/2838383002/diff/1/components/minidump_uploade... components/minidump_uploader/android/java/src/org/chromium/components/minidump_uploader/MinidumpUploaderImpl.java:159: if (mCancelUpload) return; On 2017/04/28 13:01:19, gsennton wrote: > On 2017/04/27 00:57:06, Ilya Sherman wrote: > > On 2017/04/26 13:58:36, gsennton wrote: > > > On 2017/04/26 13:41:19, Tobias Sargeant wrote: > > > > On 2017/04/26 13:34:30, Tobias Sargeant wrote: > > > > > You're leaving mUploadsDone in a potentially inconsistent state here. > > > > Thinking about it a bit more, I suspect this is unavoidable without doing > > I/O > > > > operations or blocking on the UI thread. We just have to accept, then, > that > > we > > > > may reschedule a job that will early out because it has no work to do. > > > > > > Yeah, that's the conclusion I reached as well. > > > I don't think this rescheduling is a major issue anyway - the only reason > > > getUploadsDone() would return false even when there are no minidumps left is > > the > > > case where we fail to upload some minidump, it reaches its maximum number of > > > retries, and we cancel the upload job before it calls setUploadsDone.. (if > we > > > care a lot about this case we could call setUploadsDone after each > > > minidump-upload to minimise the window during which we might incorrectly > > > schedule a new job). > > > > It's probably worth documenting this somewhere =) > > Done (I added a comment inside cancelUploads()). > > Related to this - it feels like we are adding too many comments here, not that > comments in themselves are bad - it just feels like the need of so many comments > says something about the complexity of the code :/ Yeah, I agree :/ I think most of the comments boil down to: We have unusual semantics for cancelation, and these manifest themselves in a few different spots in the code. https://codereview.chromium.org/2838383002/diff/20001/android_webview/javates... File android_webview/javatests/src/org/chromium/android_webview/test/crash/MinidumpUploaderTest.java (right): https://codereview.chromium.org/2838383002/diff/20001/android_webview/javates... android_webview/javatests/src/org/chromium/android_webview/test/crash/MinidumpUploaderTest.java:335: public void testCancelFailedUploadCausesReschedule() throws IOException { On 2017/04/28 13:01:20, gsennton wrote: > This test code is very similar (some duplication) to the tests above - but it > tests one specific thing rather than a composition of several things. I.e. this > entire test is all for the last line of method - assertTrue(cancelUploads()). > In the spirit of the doc that Ilya shared about testing at some point I think it > makes sense to duplicate the code here and just test one single thing, instead > of adding the assert to a previous test (but I might also be misunderstanding > the point here, so if you have any thoughts please do tell - I like adding lots > of tests but I often end up with a bit of a mess, which means spending more time > on test refactoring whenever the real code changes, so any feedback in this area > would be great). WDYT? Yep, I think it's better to write as two separate tests. I do think there's a lot of verbosity in the setup code, which isn't really super interesting to someone reading the test; it might be nice to hide some of that complexity in factored-out code. Ideally, the test would look something like: public void testCancelFailedUploadCausesReschedule() throws IOException { // Set up the test. MinidumpUploaderImpl uploader = new FailingMinidumpUploaderImpl(); createMinidumpFileInCrashDir("123_abc.dmp0"); // Set a mock expectation. MinidumpUploader.UploadsFinishedCallback callback = new MinidumpUploader.UploadsFinishedCallback() { @Override public void uploadsFinished(boolean reschedule) { fail("This method shouldn't be called when a canceled upload fails."); } }; // Run the test. MinidumpUploadTestUtility.uploadAllMinidumpsOnUiThread(minidumpUploader, callback); // Verify the expectations. // Ensure we tell JobScheduler to reschedule the job. assertTrue(minidumpUploader.cancelUploads()); } Maybe "FailingMinidumpUploaderImpl" is not the clearest name. And, it would be nice if the mock expectation were less verbose... it would be nice if *all* of Java were less verbose =P But I think it's still a relatively clear and relatively simple test. (I wouldn't necessarily include the "set up/run/verify" comments I added -- those were mostly to show my reasoning for how I grouped things.) https://codereview.chromium.org/2838383002/diff/20001/android_webview/javates... android_webview/javatests/src/org/chromium/android_webview/test/crash/MinidumpUploaderTest.java:364: // before we try to join it. Where do we try to join the thread? This seems like a weird implementation detail to need to call out here. https://codereview.chromium.org/2838383002/diff/20001/components/minidump_upl... File components/minidump_uploader/android/java/src/org/chromium/components/minidump_uploader/MinidumpUploaderImpl.java (right): https://codereview.chromium.org/2838383002/diff/20001/components/minidump_upl... components/minidump_uploader/android/java/src/org/chromium/components/minidump_uploader/MinidumpUploaderImpl.java:85: * Set whether we are done uploading minidumps. nit: Please update this docstring. https://codereview.chromium.org/2838383002/diff/20001/components/minidump_upl... components/minidump_uploader/android/java/src/org/chromium/components/minidump_uploader/MinidumpUploaderImpl.java:117: Log.e(TAG, "Crash directory doesn't exist!"); Optional nit: Mebbe avoid code duplication with the above if-stmt by setting an error string, and performing the logging + early return iff an error is set? https://codereview.chromium.org/2838383002/diff/20001/components/minidump_upl... components/minidump_uploader/android/java/src/org/chromium/components/minidump_uploader/MinidumpUploaderImpl.java:125: // to in case we perform any long-running operations after this (such as deleting old nit: "here to in case" -> "here in case" https://codereview.chromium.org/2838383002/diff/20001/components/minidump_upl... components/minidump_uploader/android/java/src/org/chromium/components/minidump_uploader/MinidumpUploaderImpl.java:229: // inconsistency to avoid blocking the UI-thread on IO operations. I think the important thing to call out here is that we expect to have eventual consistency: Our jobs might be rescheduled unnecessarily, but at most once.
https://codereview.chromium.org/2838383002/diff/1/components/minidump_uploade... File components/minidump_uploader/android/java/src/org/chromium/components/minidump_uploader/MinidumpUploaderImpl.java (right): https://codereview.chromium.org/2838383002/diff/1/components/minidump_uploade... components/minidump_uploader/android/java/src/org/chromium/components/minidump_uploader/MinidumpUploaderImpl.java:126: setUploadsDone(minidumps.length == 0); On 2017/04/28 22:31:15, Ilya Sherman wrote: > On 2017/04/28 13:15:47, Tobias Sargeant wrote: > > On 2017/04/28 13:01:20, gsennton wrote: > > > On 2017/04/28 10:50:17, Tobias Sargeant wrote: > > > > On 2017/04/27 00:57:06, Ilya Sherman wrote: > > > > > This call seems unnecessary. If we're done, then the for loop won't run > > > > (there > > > > > will be zero items), so we'll promptly reach the call on line 180. > Well, > > I > > > > > suppose it *could* matter if cleaning out the old files takes too long. > > If > > > > > that's the concern, then let's document that here. > > > > > > > > But it (currently) needs to be set to false here if there are minidumps to > > > > upload, so that if the job is cancelled it's left at that value, and we > can > > > > reschedule? Maybe the sense of the boolean should be inverted, so that > it's > > > > mMinidumpsToUpload instead of mUploadsDone. > > > > > > > > Actually, that's another problem: if the system cancels the job before we > > get > > > to > > > > this point, then the job won't be rescheduled. I think it's ok to set the > > > > boolean to the right sense (work to do) at the time we schedule the job, > and > > > > then only reset it once we know we've finished. > > > > > > We are already setting it to false (mUploadsDone == false means we should > > > reschedule) as default (i.e. when MinidumpUploaderImpl is created), so in > the > > > default case we choose to reschedule. > > > Or am I missing something? (we don't re-use MinidumpUploaderImpl for later > > > uploads). > > > > Then Ilya is right, we only go from false to true at the end of this function, > > and this call can be removed. > > > > Arguably, though, the logic can then be entirely removed, because the only > time > > that you would cancel and not reschedule is if the cancellation signal came > > after you left the loop. I think we could pay the extra cost of just > > rescheduling in that case, and remove mUploadsDone entirely. > > Yeah, actually, I think that's even better. Yeeepp, removed the whole mUploadsDone stuff. https://codereview.chromium.org/2838383002/diff/20001/android_webview/javates... File android_webview/javatests/src/org/chromium/android_webview/test/crash/MinidumpUploaderTest.java (right): https://codereview.chromium.org/2838383002/diff/20001/android_webview/javates... android_webview/javatests/src/org/chromium/android_webview/test/crash/MinidumpUploaderTest.java:335: public void testCancelFailedUploadCausesReschedule() throws IOException { On 2017/04/28 22:31:15, Ilya Sherman wrote: > On 2017/04/28 13:01:20, gsennton wrote: > > This test code is very similar (some duplication) to the tests above - but it > > tests one specific thing rather than a composition of several things. I.e. > this > > entire test is all for the last line of method - assertTrue(cancelUploads()). > > In the spirit of the doc that Ilya shared about testing at some point I think > it > > makes sense to duplicate the code here and just test one single thing, instead > > of adding the assert to a previous test (but I might also be misunderstanding > > the point here, so if you have any thoughts please do tell - I like adding > lots > > of tests but I often end up with a bit of a mess, which means spending more > time > > on test refactoring whenever the real code changes, so any feedback in this > area > > would be great). WDYT? > > Yep, I think it's better to write as two separate tests. I do think there's a > lot of verbosity in the setup code, which isn't really super interesting to > someone reading the test; it might be nice to hide some of that complexity in > factored-out code. Ideally, the test would look something like: > > public void testCancelFailedUploadCausesReschedule() throws IOException { > // Set up the test. > MinidumpUploaderImpl uploader = new FailingMinidumpUploaderImpl(); > createMinidumpFileInCrashDir("123_abc.dmp0"); > > // Set a mock expectation. > MinidumpUploader.UploadsFinishedCallback callback = > new MinidumpUploader.UploadsFinishedCallback() { > @Override > public void uploadsFinished(boolean reschedule) { > fail("This method shouldn't be called when a canceled upload > fails."); > } > }; > > // Run the test. > MinidumpUploadTestUtility.uploadAllMinidumpsOnUiThread(minidumpUploader, > callback); > > // Verify the expectations. > // Ensure we tell JobScheduler to reschedule the job. > assertTrue(minidumpUploader.cancelUploads()); > } > > Maybe "FailingMinidumpUploaderImpl" is not the clearest name. And, it would be > nice if the mock expectation were less verbose... it would be nice if *all* of > Java were less verbose =P But I think it's still a relatively clear and > relatively simple test. > > (I wouldn't necessarily include the "set up/run/verify" comments I added -- > those were mostly to show my reasoning for how I grouped things.) This code should now be closer to what you present in this comment :) https://codereview.chromium.org/2838383002/diff/20001/android_webview/javates... android_webview/javatests/src/org/chromium/android_webview/test/crash/MinidumpUploaderTest.java:364: // before we try to join it. On 2017/04/28 22:31:15, Ilya Sherman wrote: > Where do we try to join the thread? This seems like a weird implementation > detail to need to call out here. Yeah, it's only used in testCancellation(), it isn't needed in this test, thanks! :) https://codereview.chromium.org/2838383002/diff/20001/components/minidump_upl... File components/minidump_uploader/android/java/src/org/chromium/components/minidump_uploader/MinidumpUploaderImpl.java (right): https://codereview.chromium.org/2838383002/diff/20001/components/minidump_upl... components/minidump_uploader/android/java/src/org/chromium/components/minidump_uploader/MinidumpUploaderImpl.java:85: * Set whether we are done uploading minidumps. On 2017/04/28 22:31:15, Ilya Sherman wrote: > nit: Please update this docstring. Removed this altogether. https://codereview.chromium.org/2838383002/diff/20001/components/minidump_upl... components/minidump_uploader/android/java/src/org/chromium/components/minidump_uploader/MinidumpUploaderImpl.java:117: Log.e(TAG, "Crash directory doesn't exist!"); On 2017/04/28 22:31:15, Ilya Sherman wrote: > Optional nit: Mebbe avoid code duplication with the above if-stmt by setting an > error string, and performing the logging + early return iff an error is set? The CrashFileManager ctor (called from createCrashFileManager) needs the crash-parent directory to exist though, that's why this code looks a bit icky :/ https://codereview.chromium.org/2838383002/diff/20001/components/minidump_upl... components/minidump_uploader/android/java/src/org/chromium/components/minidump_uploader/MinidumpUploaderImpl.java:125: // to in case we perform any long-running operations after this (such as deleting old On 2017/04/28 22:31:15, Ilya Sherman wrote: > nit: "here to in case" -> "here in case" Removed setUploadsDone() altogether. https://codereview.chromium.org/2838383002/diff/20001/components/minidump_upl... components/minidump_uploader/android/java/src/org/chromium/components/minidump_uploader/MinidumpUploaderImpl.java:229: // inconsistency to avoid blocking the UI-thread on IO operations. On 2017/04/28 22:31:15, Ilya Sherman wrote: > I think the important thing to call out here is that we expect to have eventual > consistency: Our jobs might be rescheduled unnecessarily, but at most once. Done.
LGTM. Thanks, Gustav!
lgtm
The CQ bit was checked by gsennton@chromium.org
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
The CQ bit was checked by gsennton@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from isherman@chromium.org, tobiasjs@chromium.org Link to the patchset: https://codereview.chromium.org/2838383002/#ps100001 (title: "Don't fail test if uploadFinished called - we don't guarantee ng.")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 100001, "attempt_start_ts": 1493814734785830,
"parent_rev": "c970a008d88fdabd03b770412446b3dc8c5e8b63", "commit_rev":
"ee63feacd7199214bf9673efd957afabe9be13bb"}
Message was sent while issue was closed.
Description was changed from ========== Move Minidump Uploading IO operations off the UI thread. Before this CL we would create a CrashFileManager in MinidumpUploaderImpl on the UI thread. CrashFileManager requires a directory in which to store minidumps, so creating that directory, or ensuring that it exists, requires disk operations. In this CL we move the creation of the CrashFileManager onto the worker thread where the actual minidump uploading happens. Also change the rescheduling logic for minidump uploading slightly, Previously we would check whether there are any minidumps left, at the time when the current job is cancelled, from the UI thread. But that requires a disk operation, so now we instead update whether to reschedule uploads, from the worker thread, for the UI thread to read in a thread-safe way (without disk reads). Note that this change affects the minidump uploading of both WebView and Chrome. BUG=714138 ========== to ========== Move Minidump Uploading IO operations off the UI thread. Before this CL we would create a CrashFileManager in MinidumpUploaderImpl on the UI thread. CrashFileManager requires a directory in which to store minidumps, so creating that directory, or ensuring that it exists, requires disk operations. In this CL we move the creation of the CrashFileManager onto the worker thread where the actual minidump uploading happens. Also change the rescheduling logic for minidump uploading slightly, Previously we would check whether there are any minidumps left, at the time when the current job is cancelled, from the UI thread. But that requires a disk operation, so now we instead update whether to reschedule uploads, from the worker thread, for the UI thread to read in a thread-safe way (without disk reads). Note that this change affects the minidump uploading of both WebView and Chrome. BUG=714138 Review-Url: https://codereview.chromium.org/2838383002 Cr-Commit-Position: refs/heads/master@{#468955} Committed: https://chromium.googlesource.com/chromium/src/+/ee63feacd7199214bf9673efd957... ==========
Message was sent while issue was closed.
Committed patchset #6 (id:100001) as https://chromium.googlesource.com/chromium/src/+/ee63feacd7199214bf9673efd957... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
