Consolidated patch set from bug 1122698, bug 1134527, bug 1134529, bug 1174500
Categories
(MailNews Core :: Networking, defect)
Tracking
(Not tracked)
People
(Reporter: ishikawa, Assigned: ishikawa, NeedInfo)
References
(Blocks 4 open bugs)
Details
Attachments
(33 files, 90 obsolete files)
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
application/pdf
|
Details | |
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
text/plain
|
Details | |
(deleted),
text/plain
|
Details | |
(deleted),
image/png
|
Details | |
(deleted),
image/png
|
Details | |
(deleted),
image/png
|
Details | |
(deleted),
image/png
|
Details | |
(deleted),
application/zip
|
Details | |
(deleted),
application/zip
|
Details | |
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
text/plain
|
Details | |
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
patch
|
benc
:
feedback+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
patch
|
Details | Diff | Splinter Review |
Assignee | ||
Comment 1•9 years ago
|
||
Assignee | ||
Updated•9 years ago
|
Assignee | ||
Comment 2•9 years ago
|
||
Assignee | ||
Comment 3•9 years ago
|
||
Assignee | ||
Comment 4•9 years ago
|
||
Assignee | ||
Updated•9 years ago
|
Assignee | ||
Updated•9 years ago
|
Assignee | ||
Comment 5•9 years ago
|
||
Assignee | ||
Updated•9 years ago
|
Assignee | ||
Updated•9 years ago
|
Assignee | ||
Updated•9 years ago
|
Assignee | ||
Updated•9 years ago
|
Assignee | ||
Updated•9 years ago
|
Assignee | ||
Comment 6•9 years ago
|
||
Assignee | ||
Comment 7•9 years ago
|
||
Assignee | ||
Comment 8•9 years ago
|
||
Assignee | ||
Comment 9•9 years ago
|
||
Assignee | ||
Comment 10•9 years ago
|
||
Assignee | ||
Comment 11•9 years ago
|
||
Assignee | ||
Comment 12•8 years ago
|
||
Comment 13•8 years ago
|
||
Assignee | ||
Comment 14•8 years ago
|
||
Comment 15•8 years ago
|
||
Assignee | ||
Comment 16•8 years ago
|
||
Assignee | ||
Comment 17•8 years ago
|
||
Assignee | ||
Comment 18•8 years ago
|
||
Assignee | ||
Comment 19•8 years ago
|
||
Assignee | ||
Comment 20•8 years ago
|
||
Assignee | ||
Comment 21•8 years ago
|
||
Assignee | ||
Comment 25•8 years ago
|
||
Assignee | ||
Comment 26•8 years ago
|
||
Assignee | ||
Comment 27•8 years ago
|
||
Assignee | ||
Comment 28•8 years ago
|
||
Comment 31•8 years ago
|
||
Comment 32•8 years ago
|
||
Assignee | ||
Comment 33•8 years ago
|
||
Assignee | ||
Comment 34•8 years ago
|
||
Assignee | ||
Comment 35•8 years ago
|
||
Assignee | ||
Comment 36•8 years ago
|
||
Assignee | ||
Comment 37•8 years ago
|
||
Assignee | ||
Comment 38•8 years ago
|
||
Assignee | ||
Comment 39•8 years ago
|
||
Assignee | ||
Updated•8 years ago
|
Comment 40•8 years ago
|
||
Assignee | ||
Comment 41•8 years ago
|
||
Assignee | ||
Comment 42•8 years ago
|
||
Assignee | ||
Comment 43•8 years ago
|
||
Assignee | ||
Comment 44•8 years ago
|
||
Assignee | ||
Comment 45•8 years ago
|
||
Assignee | ||
Comment 46•8 years ago
|
||
Assignee | ||
Comment 47•8 years ago
|
||
Assignee | ||
Comment 48•8 years ago
|
||
Comment 49•8 years ago
|
||
Assignee | ||
Comment 50•8 years ago
|
||
Comment 51•8 years ago
|
||
Assignee | ||
Comment 52•8 years ago
|
||
Comment 53•8 years ago
|
||
Comment 54•8 years ago
|
||
Comment 55•8 years ago
|
||
Assignee | ||
Comment 56•8 years ago
|
||
Comment 57•8 years ago
|
||
Assignee | ||
Comment 58•8 years ago
|
||
Assignee | ||
Comment 59•8 years ago
|
||
Assignee | ||
Comment 60•8 years ago
|
||
Comment 61•8 years ago
|
||
Comment 62•8 years ago
|
||
Comment 63•8 years ago
|
||
Assignee | ||
Comment 64•8 years ago
|
||
Assignee | ||
Comment 65•8 years ago
|
||
Assignee | ||
Comment 66•8 years ago
|
||
Comment 67•8 years ago
|
||
Comment 68•8 years ago
|
||
Assignee | ||
Updated•8 years ago
|
Assignee | ||
Comment 69•8 years ago
|
||
Assignee | ||
Updated•8 years ago
|
Assignee | ||
Updated•8 years ago
|
Assignee | ||
Updated•8 years ago
|
Assignee | ||
Updated•8 years ago
|
Assignee | ||
Updated•8 years ago
|
Assignee | ||
Updated•8 years ago
|
Assignee | ||
Comment 70•8 years ago
|
||
Assignee | ||
Updated•8 years ago
|
Assignee | ||
Comment 71•8 years ago
|
||
Comment 72•8 years ago
|
||
Assignee | ||
Comment 73•8 years ago
|
||
Comment 74•8 years ago
|
||
Comment 75•8 years ago
|
||
Comment 76•8 years ago
|
||
Comment 77•8 years ago
|
||
Assignee | ||
Comment 78•8 years ago
|
||
Assignee | ||
Comment 79•8 years ago
|
||
Comment 80•8 years ago
|
||
Updated•8 years ago
|
Assignee | ||
Comment 81•8 years ago
|
||
Comment 82•8 years ago
|
||
Assignee | ||
Comment 83•8 years ago
|
||
Assignee | ||
Comment 84•8 years ago
|
||
Assignee | ||
Comment 85•8 years ago
|
||
Assignee | ||
Comment 86•8 years ago
|
||
Assignee | ||
Comment 87•8 years ago
|
||
Assignee | ||
Comment 88•8 years ago
|
||
Comment 89•8 years ago
|
||
Assignee | ||
Comment 90•8 years ago
|
||
Assignee | ||
Comment 91•8 years ago
|
||
Assignee | ||
Updated•8 years ago
|
Assignee | ||
Comment 92•8 years ago
|
||
Assignee | ||
Comment 93•8 years ago
|
||
Assignee | ||
Comment 94•8 years ago
|
||
Assignee | ||
Updated•8 years ago
|
Assignee | ||
Comment 95•8 years ago
|
||
Assignee | ||
Comment 96•8 years ago
|
||
Comment 97•8 years ago
|
||
Comment 98•8 years ago
|
||
Assignee | ||
Comment 99•8 years ago
|
||
Assignee | ||
Comment 100•8 years ago
|
||
Assignee | ||
Comment 101•8 years ago
|
||
Assignee | ||
Comment 102•8 years ago
|
||
Assignee | ||
Comment 103•8 years ago
|
||
Assignee | ||
Comment 104•8 years ago
|
||
Comment 105•8 years ago
|
||
Assignee | ||
Comment 106•8 years ago
|
||
Comment 107•8 years ago
|
||
Assignee | ||
Comment 108•8 years ago
|
||
Assignee | ||
Comment 109•8 years ago
|
||
Assignee | ||
Comment 110•8 years ago
|
||
Assignee | ||
Comment 111•8 years ago
|
||
Assignee | ||
Comment 112•8 years ago
|
||
Assignee | ||
Comment 113•8 years ago
|
||
Assignee | ||
Updated•8 years ago
|
Comment 114•8 years ago
|
||
Comment 115•8 years ago
|
||
Assignee | ||
Comment 116•8 years ago
|
||
Assignee | ||
Comment 117•8 years ago
|
||
Assignee | ||
Comment 118•8 years ago
|
||
Updated•8 years ago
|
Updated•8 years ago
|
Comment 119•8 years ago
|
||
Comment 120•8 years ago
|
||
Comment 121•8 years ago
|
||
Comment 122•8 years ago
|
||
Comment 123•8 years ago
|
||
Assignee | ||
Comment 124•8 years ago
|
||
Comment 125•8 years ago
|
||
Comment 126•8 years ago
|
||
Comment 127•8 years ago
|
||
Comment 128•8 years ago
|
||
Assignee | ||
Comment 129•8 years ago
|
||
Comment 130•8 years ago
|
||
Assignee | ||
Comment 131•8 years ago
|
||
Comment 132•8 years ago
|
||
Assignee | ||
Comment 133•8 years ago
|
||
Comment 134•8 years ago
|
||
Comment 135•8 years ago
|
||
Comment 136•8 years ago
|
||
Comment 137•8 years ago
|
||
Comment 138•8 years ago
|
||
Assignee | ||
Comment 139•8 years ago
|
||
Comment 140•8 years ago
|
||
Assignee | ||
Comment 141•8 years ago
|
||
Assignee | ||
Comment 142•8 years ago
|
||
Assignee | ||
Comment 143•8 years ago
|
||
Assignee | ||
Comment 144•8 years ago
|
||
Assignee | ||
Comment 145•8 years ago
|
||
Assignee | ||
Updated•8 years ago
|
Assignee | ||
Comment 146•8 years ago
|
||
Comment 147•8 years ago
|
||
Assignee | ||
Comment 148•8 years ago
|
||
Assignee | ||
Comment 149•8 years ago
|
||
Assignee | ||
Comment 150•8 years ago
|
||
Assignee | ||
Comment 151•7 years ago
|
||
Assignee | ||
Comment 152•7 years ago
|
||
Assignee | ||
Comment 153•7 years ago
|
||
Assignee | ||
Comment 154•7 years ago
|
||
Assignee | ||
Comment 155•7 years ago
|
||
Assignee | ||
Comment 156•7 years ago
|
||
Assignee | ||
Comment 157•7 years ago
|
||
Assignee | ||
Comment 158•7 years ago
|
||
Assignee | ||
Comment 159•7 years ago
|
||
Assignee | ||
Comment 160•7 years ago
|
||
Assignee | ||
Comment 161•7 years ago
|
||
Assignee | ||
Comment 162•7 years ago
|
||
Assignee | ||
Comment 163•7 years ago
|
||
Assignee | ||
Comment 164•7 years ago
|
||
Assignee | ||
Comment 165•7 years ago
|
||
Assignee | ||
Comment 166•7 years ago
|
||
Assignee | ||
Comment 167•7 years ago
|
||
Assignee | ||
Comment 168•7 years ago
|
||
Assignee | ||
Comment 169•7 years ago
|
||
Assignee | ||
Comment 170•7 years ago
|
||
Assignee | ||
Comment 171•7 years ago
|
||
Assignee | ||
Comment 172•7 years ago
|
||
Comment 173•7 years ago
|
||
Assignee | ||
Comment 174•7 years ago
|
||
Comment 175•7 years ago
|
||
Assignee | ||
Comment 176•7 years ago
|
||
Comment 177•7 years ago
|
||
Assignee | ||
Comment 178•7 years ago
|
||
Assignee | ||
Comment 179•7 years ago
|
||
Assignee | ||
Comment 180•7 years ago
|
||
Assignee | ||
Comment 181•7 years ago
|
||
Assignee | ||
Comment 182•7 years ago
|
||
Assignee | ||
Comment 183•7 years ago
|
||
Comment 184•7 years ago
|
||
Assignee | ||
Comment 185•7 years ago
|
||
Assignee | ||
Comment 186•7 years ago
|
||
Assignee | ||
Comment 187•7 years ago
|
||
Assignee | ||
Comment 188•7 years ago
|
||
Assignee | ||
Comment 189•7 years ago
|
||
Assignee | ||
Comment 190•7 years ago
|
||
Comment 191•7 years ago
|
||
Assignee | ||
Comment 192•7 years ago
|
||
Assignee | ||
Comment 193•7 years ago
|
||
Assignee | ||
Comment 194•7 years ago
|
||
Assignee | ||
Comment 195•7 years ago
|
||
Assignee | ||
Comment 196•7 years ago
|
||
Assignee | ||
Comment 197•7 years ago
|
||
Assignee | ||
Comment 198•7 years ago
|
||
Assignee | ||
Comment 199•7 years ago
|
||
Comment 200•7 years ago
|
||
Assignee | ||
Comment 201•7 years ago
|
||
Assignee | ||
Comment 202•7 years ago
|
||
Assignee | ||
Comment 203•7 years ago
|
||
Comment 204•7 years ago
|
||
Assignee | ||
Comment 205•7 years ago
|
||
Assignee | ||
Comment 206•7 years ago
|
||
Assignee | ||
Comment 207•7 years ago
|
||
Comment 208•7 years ago
|
||
Comment 209•7 years ago
|
||
Assignee | ||
Comment 210•7 years ago
|
||
Comment 211•7 years ago
|
||
Comment 212•7 years ago
|
||
Comment 213•7 years ago
|
||
Assignee | ||
Comment 214•7 years ago
|
||
Comment 215•7 years ago
|
||
Comment 216•7 years ago
|
||
Comment 217•7 years ago
|
||
Comment 218•7 years ago
|
||
Comment 219•7 years ago
|
||
Assignee | ||
Comment 220•7 years ago
|
||
Comment 221•7 years ago
|
||
Comment 222•7 years ago
|
||
Comment 223•7 years ago
|
||
Comment 224•7 years ago
|
||
Assignee | ||
Comment 225•7 years ago
|
||
Comment 226•7 years ago
|
||
Comment 227•7 years ago
|
||
Comment 228•7 years ago
|
||
Assignee | ||
Comment 230•7 years ago
|
||
Comment 231•7 years ago
|
||
Assignee | ||
Comment 232•7 years ago
|
||
Assignee | ||
Comment 233•7 years ago
|
||
Assignee | ||
Comment 234•6 years ago
|
||
Assignee | ||
Comment 235•5 years ago
|
||
I am going to upload a new set of patches that are now compatible with reformatted source tree, and are known to work.
(Finally, I could get rid of a couple of errors that showed up: one in linux and OSX build, and additional one in Windows build on tryserver tests.
After carefuly checking I found that there was a subtle copy&paste error during the update to match clang-formatted source tree.
All is well : passes mozmil and xpcshell-test.
Assignee | ||
Comment 236•5 years ago
|
||
It ripped out the unnecessary caching as far as the buffered writing is concerned. (That is, as far as the code I touched is concerned. Maybe not everywhere. It may have to be touched in another buzilla comprehensively IMHO.)
Assignee | ||
Comment 237•5 years ago
|
||
The following patches will be uploaded:
1242030-DONT-USE-REUSABLE-new-part-1.patch: bug 1242030: DONT-USE-REUSABLE new part-1 new splitting of (consolidation of 1122698, 1134527, 1134529, 1174500)
1242030-DONT-USE-REUSABLE-new-part-2-imap-JK-v1-rev02.patch: bug 1242030: DONT-USE-REUSABLE Improve error handling in file/stream I/O. Part 2. r=jorgk.
1242030-DONT-USE-REUSABLE-new-part-3-import-JK-v1-rev03.patch: bug 1242030: DONT-USE-REUSABLE Improve error handling in file/stream I/O. Part 3 (was 2). r=jorgk. revision 2
1242030-DONT-USE-REUSABLE-new-part-4-local-less-pop3.patch: bug 1242030: DONT-USE-REUSABLE new part-4 new splitting of (consolidation of 1122698, 1134527, 1134529, 1174500)
1242030-DONT-USE-REUSABLE-new-part-5-pop3.patch: bug 1242030: DONT-USE-REUSABLE new part-5 new splitting of (consolidation of 1122698, 1134527, 1134529, 1174500)
Additionally, following two patches posted in respective bugzilla completes the buffered output finally (!).
enable-buffering-1st-step.patch: Bug 1242042: Enabling buffering for file stream to write message for C-C TB (Enabling buffering first step.)
removing-a-Seek-rev02.patch: Bug 1242046 - Removing unnecessary |Seek| that caused the C-C TB to operate slowly in terms of I/O
Assignee | ||
Comment 238•5 years ago
|
||
clang formatted. I obsoleted attachment 8879215 [details] [diff] [review].
Assignee | ||
Comment 239•5 years ago
|
||
clang-formatted. I obsoleted attachment 8879217 [details] [diff] [review] (1242030-DONT-USE-REUSABLE-new-part-2-imap-JK-v1-rev02.patch)
Assignee | ||
Comment 240•5 years ago
|
||
clang-formatted. I obsoleted 8879218: 1242030-DONT-USE-REUSABLE-new-part-3-import-JK-v1-rev02.patch.
Assignee | ||
Comment 241•5 years ago
|
||
clang-formatted. I obsoleted 8879219: 1242030-DONT-USE-REUSABLE-new-part-4-local-less-pop3.patch.
Assignee | ||
Comment 242•5 years ago
|
||
clang-formatted. I obsoleted 8879221: 1242030-DONT-USE-REUSABLE-new-part-5-pop3.patch.
Assignee | ||
Comment 243•5 years ago
|
||
Folks,
Can somebody give me an idea how I should go about landing these patches?
It has taken so many years and I am a bit at a loss myself.
I am cc: ing to Jorg, aceman and wayne for now.
Thank you in advance.
Comment 244•5 years ago
|
||
Hi, thank you. Yes, it is a pity this took so long, but if the patch set works now, we should be able to accept it.
You did not obsolete some of the older patch sets named "USE REUSABLE" and "USE CLOSED". Are those still valid in addition to the new "DONT USE REUSABLE" patch set?
Comment 245•5 years ago
|
||
Thanks for continuing to push this through. I defer to Jorg in these code matters. I look forward to seeing this in a release!
Assignee | ||
Comment 246•5 years ago
|
||
(In reply to :aceman from comment #244)
Hi, thank you. Yes, it is a pity this took so long, but if the patch set works now, we should be able to accept it.
Thank you for you comment.
That will be great, but we need to review them first.
(There were a few modifications that were necessary to get the local compiler and tryserver MS STUDIO compiler build the code.
The latter happened in the summer of 2017 for about 10 days if I recall correctly.)
You did not obsolete some of the older patch sets named "USE REUSABLE" and "USE CLOSED". Are those still valid in addition to the new "DONT USE REUSABLE" patch set?
Well, they were referred in the old comments and so I thought it might be necessary to keep them.
Maybe I should obsolete them to avoid misunderstanding. I will do so after this comment.
BTW, as the latest patch name suggests, the patch set does away with reusable flag based on my observation reported here (comment 234) that caching of file stream is not worthwhile and my patch that reduces the # of write system calls is a big win for copying messages between folders even.
(In reply to Wayne Mery (:wsmwk) from comment #245)
Thanks for continuing to push this through. I defer to Jorg in these code matters. I look forward to seeing this in a release!
Yup. As Jorg is one of the key maintainers of C-C TB , I need his input to keep the code in good shape.
So a review cycle will be necessary before landing.
I will do more local testing (using a router's USB stick as remote server storage and unplugging the network cable (simulated by disabling networking of VirtualBox) while the review will go on.
TIA
Assignee | ||
Comment 247•5 years ago
|
||
I am obsoleting these patches.
(They may be referenced in comments, but I think we can access obsoleted attachments if necessary.)
USE-REUSABLE 1242030-new-part-1.patch
USE REUSABLE 1242030-new-part-2-imap.patch
USE REUSABLE 1242030-new-part-3-import.patch
USE REUSABLE 1242030-new-part-4-local-less-pop3.patch
USE REUSABLE 1242030-new-part-5-pop3.patch
USE REUSABLE 1242030-part0-pass-out-reusable.patch
Hmm... I should have added updated -rev03 suffix to my latest patches. I would do so locally now.
1242030-USE-CLOSED-new-part-1.patch
1242030-USE-CLOSED-new-part-2-imap-JK-v1-rev02.patch
1242030-USE-CLOSED-new-part-3-import-JK-v1-rev02.patch
1242030-USE-CLOSED-new-part-4-local-less-pop3.patch
1242030-USE-CLOSED-new-part-5-pop3.patch
Assignee | ||
Updated•5 years ago
|
Assignee | ||
Updated•5 years ago
|
Assignee | ||
Updated•5 years ago
|
Assignee | ||
Updated•5 years ago
|
Assignee | ||
Updated•5 years ago
|
Assignee | ||
Updated•5 years ago
|
Assignee | ||
Updated•5 years ago
|
Assignee | ||
Updated•5 years ago
|
Assignee | ||
Updated•5 years ago
|
Assignee | ||
Updated•5 years ago
|
Assignee | ||
Updated•5 years ago
|
Assignee | ||
Comment 248•5 years ago
|
||
Done.
(In reply to ISHIKAWA, Chiaki from comment #247)
I am obsoleting these patches.
(They may be referenced in comments, but I think we can access obsoleted attachments if necessary.)USE-REUSABLE 1242030-new-part-1.patch
USE REUSABLE 1242030-new-part-2-imap.patch
USE REUSABLE 1242030-new-part-3-import.patch
USE REUSABLE 1242030-new-part-4-local-less-pop3.patch
USE REUSABLE 1242030-new-part-5-pop3.patch
USE REUSABLE 1242030-part0-pass-out-reusable.patchHmm... I should have added updated -rev03 suffix to my latest patches. I would do so locally now.
I meant to say that I should update the existing -rev02 to -rev03 and then add -rev03 to patch names that don't have them.
Already a couple of them do have such -rev03 string, but no all of them yet in the attachments.
1242030-USE-CLOSED-new-part-1.patch
1242030-USE-CLOSED-new-part-2-imap-JK-v1-rev02.patch
1242030-USE-CLOSED-new-part-3-import-JK-v1-rev02.patch
1242030-USE-CLOSED-new-part-4-local-less-pop3.patch
1242030-USE-CLOSED-new-part-5-pop3.patch
Comment 249•5 years ago
|
||
Can somebody give me an idea how I should go about landing these patches?
They need to be reviewed again. It would also be good to see the performance comparison again. Maybe we should get our new backend man, Ben C, involved, although he's generally quite busy.
Assignee | ||
Comment 250•5 years ago
|
||
(In reply to Jorg K (GMT+2) from comment #249)
Can somebody give me an idea how I should go about landing these patches?
They need to be reviewed again.
Sure.
Actually, I would appreciate it.
Due to some compiler issues locally and on tryserver, I had to re-do some parts of earlier code which you kindly reviewed before.
So we have to make sure the code is in good shape in terms of today's debugging macros, etc.
It would also be good to see the performance comparison again. Maybe we should get our new backend man, Ben C, involved, although he's generally quite busy.
I can do the comparison again.
So can you review the patches at least partially, Jorg?
TIA
Comment 251•5 years ago
|
||
Assignee | ||
Comment 252•5 years ago
|
||
(In reply to Jorg K (GMT+2) from comment #251)
Comment on attachment 9106025 [details] [diff] [review]
1242030-DONT-USE-REUSABLE-new-part-1.patch (clang-formatted)Review of attachment 9106025 [details] [diff] [review]:
I looked through the first patch. Let's try to get all the formal stuff and
nits out of the way first.::: mailnews/base/public/nsIMsgPluggableStore.idl
@@ +16,5 @@interface nsIUrlListener;
interface nsIMsgDatabase;
interface nsITransaction;+[scriptable, uuid(046AB3DC-D4C7-11E5-95DB-0800272954B9)]
We don't change UUIDs any more.
So it can remain the same even if the argument list of a public function (the removal of |aReusable| happened (?)
@@ +120,5 @@
+// * @param aReusable set to true on output if the caller can reuse the
+// * stream for multiple messages, e.g., mbox format.
+// * This means the caller will likely get the same stream
+// * back on multiple calls to this method, and shouldn't
+// * close the stream in between calls if they want reuse.Remove old comment.
Will do.
@@ +135,5 @@
* to an other folder as a filter action, or is deleted because it's * a duplicate. This gives the berkeley mailbox store a chance to simply * truncate the Inbox w/o leaving a deleted message in the store. *
- discardNewMessage closes aOutputStream always unless the passed stream
Trailing space.
My. I thought clang-format will take care of extra trailing whitespace automagically.
This means I may still have other trailing whatspace stuff in the patches.
::: mailnews/base/util/nsMsgDBFolder.cpp
@@ +729,5 @@
- if (NS_FAILED(rv)) {
+#ifdef DEBUG- fprintf(stderr,
"(debug) nsMsgDBFolder::GetOfflineFileStream: "
"GetMsgInputStream(hdr, &reusable, aFileStream); rv=0x%" PRIx32
that should be rv=%" PRIx32.
OK, I thought I need to add 0x in front of %. As you already noticed below, I got confused above.
Will fix these through out the patch set.
@@ +835,5 @@
+#ifdef DEBUG
- if (NS_FAILED(rv)) {
- fprintf(stderr,
"(debug) nsMsgDBFolder::GetMsgInputStream: msgStore->"
"GetMsgInputStream(this, ...) returned error rv=%" PRIx32 "\n",
Correct.
@@ +936,5 @@
nsresult rv = OpenBackupMsgDatabase();
+#ifdef DEBUG
- if (NS_FAILED(rv)) {
- fprintf(stderr,
"(debug) OpenBackupMsgDatabase(); returns error=0x%" PRIx32 "\n",
Wrong.
Will fix this and friends.
@@ +1668,5 @@
NS_WARNING("m_tempMessageStream->close returned error");
+#ifdef DEBUG
fprintf(stderr,
"(debug) Offline message too small: messageSize=%d "
"curStorePos=%" PRId64 " numOfflineMsgLines=%d bytesAdded=%d\n",
Are the %d right for all platforms?
As far as I know, I didn't get compiler errors and I thought %d were all right.
(I thought the # of lines and bytes added for a single message are 32-bit entities.)
But let me check. In the worst case scenario, I can extend the size on all platforms and use PRId64, but that seems to be a bit far-fetched.
TIA
Assignee | ||
Comment 253•5 years ago
|
||
By the way, the successful build run on tryserver with the patch set and a few additional ones [LDAP library reformat, etc.] is, for example,
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=ffebf8451c3ba83e3e6f53b198b861e7251ace26
[I have run them with a minor modification and additional local patches about a dozen times by now.]
TIA
Assignee | ||
Comment 254•5 years ago
|
||
Jorg, I thought I would take care of the following in one sweep.
that should be rv=%" PRIx32.
But then I realized that I added "0x" in front of % to obtain a 0x prefix for the printed error value.
I see output of error value as hexadecimal value as in the following example from error handling macros:
0:01.52 pid:27621 [27621, Main Thread] WARNING: NS_ENSURE_SUCCESS(rv, kKnownEsrVersion) failed with result 0x80004002: file /NEW-SSD/NREF-COMM-CENTRAL/mozilla/toolkit/components/resistfingerprinting/nsRFPService.cpp, line 661
The value 80004002 is prefixed by "0x" to signal it is a hexadecimal number.
For example, the macro NS_ENSURE_SUCCESS_BODY is define thusly.
https://dxr.mozilla.org/mozilla-central/source/xpcom/base/nsDebug.h#250
define NS_ENSURE_SUCCESS_BODY(res, ret) \
mozilla::SmprintfPointer msg = mozilla::Smprintf( \
"NS_ENSURE_SUCCESS(%s, %s) failed with " \
"result 0x%" PRIX32, \
#res, #ret, static_cast<uint32_t>(__rv)); \
NS_WARNING(msg.get());
I think I added "0x" before "%" for the same reason.
That there are a few missing cases need fixing.
Am I mistaken for the use of PRIx32 here?
TIA
PS: will check if %d is good for all architecture later today.
Assignee | ||
Comment 255•5 years ago
|
||
As for the trailing space issue, I am not entirely sure if I accidentally add it after clang-format took care of the reformatting, or if clang-format honors the trailing space inside comment block. Could be the latter case.
Assignee | ||
Updated•5 years ago
|
Assignee | ||
Comment 256•5 years ago
|
||
Ouch, I now recall Jorg reads the messages sent automatically for C-C TB bugzilla filings.
How do I turn off the needinfo I turned on.
Oh, I can provide the information and then turn off the needinfo flag. Hmm, somewhat unintuitive, but hope it works.
Assignee | ||
Comment 257•5 years ago
|
||
(In reply to Jorg K (GMT+2) from comment #251)
Comment on attachment 9106025 [details] [diff] [review]
1242030-DONT-USE-REUSABLE-new-part-1.patch (clang-formatted)
[omission]
>
> @@ +1668,5 @@
> > + NS_WARNING("m_tempMessageStream->close returned error");
> > +#ifdef DEBUG
> > + fprintf(stderr,
> > + "(debug) Offline message too small: messageSize=%d "
> > + "curStorePos=%" PRId64 " numOfflineMsgLines=%d bytesAdded=%d\n",
>
> Are the %d right for all platforms?
I checked. It should be OK.
Here the data types are all 32-bit except for curStorePos which is int64_t.
So use of %d is OK. See below.
(defined in the same function)
uint32_t messageSize;
int64_t curStorePos;
comm/mailnews/base/util/nsMsgDBFolder.h
201 int32_t m_numOfflineMsgLines;
omm/mailnews/base/util/nsMsgDBFolder.h
202 int32_t m_bytesAddedToLocalMsg;
So the patch should be OK.
TIA
Assignee | ||
Comment 258•5 years ago
|
||
As for redoing the performance analysis of copying messages between folders (as reported in comment 234), I think I need about 10 days to do the comprehensive analysis.
(Given that a large technological exhibition for which I work as part of an organizer and exhibitor side as well is fast approaching [it is slated in Dec 11-13], I may not be able to perform the comprehensive test until December 20th in the worst case, but I am trying to modify the patch as reviewed more or less in realtime.)
Comment 259•5 years ago
|
||
If the variables are int32_t then use PRId32 in the format string to be safe. PRIu32 for the uint32_t variable.
Assignee | ||
Comment 260•5 years ago
|
||
(In reply to :aceman from comment #259)
If the variables are int32_t then use PRId32 in the format string to be safe. PRIu32 for the uint32_t variable.
Oh, I did not realize that. I will modify the patch.
I am curious, though. Which architecture has an issue with %d to print int32_t and/or uint32_t?
Assignee | ||
Comment 261•5 years ago
|
||
Updated to accommodate Jorg's comment.
Uses PRIu32 and PRId32 to print out unsigned and signed 32-bit int.
Generic description for the the five (5) patches:
There were mixed usage of "0x%" PRIx32 and "..%" PRIx32 (the latter not preceded by 0x).
But my intention was to precede the hex printout of nsresult value.
So I changed them all to "0x%" PRIx32.
The remaining "%d" usage is for printing the value of LINE macro, which is an integer (and my cursory inspection indicates they are 16 bit entities .
So I think it should not be a big problem.
I replaced |-rev03| with |-rev04| the patch file name.
Assignee | ||
Comment 262•5 years ago
|
||
See previous upload.
Assignee | ||
Comment 263•5 years ago
|
||
See part-1 of the series.
Assignee | ||
Updated•5 years ago
|
Assignee | ||
Comment 264•5 years ago
|
||
See part-1 of the patch series.
Assignee | ||
Comment 266•5 years ago
|
||
I forgot to mention. UUID was not reverted yet because I was not sure if I can retain the same UUID even when the published API function has a different set of parameters.
TIA
Comment 267•5 years ago
|
||
UUIDs never need to change nowadays. It was only needed for binary add-ons which are long gone.
Assignee | ||
Comment 268•5 years ago
|
||
I reverted the UUID back to the original value.
Otherwise, the same with the previous version.
Updated•5 years ago
|
Assignee | ||
Comment 269•4 years ago
|
||
I will update the patches mentioned in comment 237 with the latest local patches.
And do the performance measurement against the current distributed binary (on linux probably.)
Also, I may want to add a few cleanups JorgK mentioned when we don't need to use reuseflag.
Assignee | ||
Comment 270•3 years ago
|
||
I will refresh the patches to fix bitrot.
(I was side tracked with other minor bugs and valgrind not usable against TB under linux for the better part of past 16 months.)
Assignee | ||
Updated•3 years ago
|
Assignee | ||
Comment 271•2 years ago
|
||
These patches are updated with recent C-C tree changes, and they
- removed the use of reuse flag as far as message handlings are concernted, and
- implement the old legacy pop3 error recovery more or less correctly (more on this later).
I will upload the updated performance figure(s) similar to those were posted before the patch.
From my local patch queue:
4 A new-1242030-precond-000-nsMsgUtils.h-typo-fix.patch: Fixed a typo and added file stream tracing macros
5 A new-1242030-DONT-USE-REUSABLE-new-part-1-rev04-with-the-original-uuid.patch: bug 1242030: DONT-USE-REUSABLE new part-1 new splitti...
6 A new-1242030-DONT-USE-REUSABLE-new-part-1.5-mostly-remove-reuseflag.patch: bug 1242030: DONT-USE-REUSABLE new part-1.5 mostly remov...
7 A new-1242030-very-tell-removal-was-OK-1719996.patch: check whether removing tell was OK.
8 A new-1242030-DONT-USE-REUSABLE-new-part-2-imap-JK-v1-rev04.patch: bug 1242030: DONT-USE-REUSABLE Improve error handling in file/str...
9 A new-1242030-DONT-USE-REUSABLE-new-part-3-import-JK-v1-rev04.patch: bug 1242030: DONT-USE-REUSABLE Improve error handling in file/s...
10 A new-1242030-DONT-USE-REUSABLE-new-part-4-local-less-pop3-rev04.patch: bug 1242030: DONT-USE-REUSABLE new part-4 new splitting of (...
11 A new-1242030-DONT-USE-REUSABLE-new-part-5-pop3-rev04.patch: bug 1242030: DONT-USE-REUSABLE new part-5 new splitting of (consolidati...
12 A new-1242030-enable-buffering-1st-step-rev03.patch: Bug 1242042: Enabling buffering for file stream to write message for C-C TB (En...
13 A new-1242030-removing-a-Seek-rev03.patch: Bug 1242046 - Removing unnecessary |Seek| that caused the C-C TB to operate slowly in ter...
14 A new-1242030-disable-safe-stream-base-search-local.patch:
15 U add-stream-tracing-macros.patch: adding stream value tracing macros
16 U add-trace-local-pop3.patch: adding stream trace macro to pop3 files under local directory
17 U add-trace-local-misc.patch: adding stream trace macro calls to files under loca (not pop3)
18 U add-stream-trace-to-base-dir-rev01.patch: add stream trace macro invocations to files under 'base' dir
19 U add-stream-trace-to-imap-dir-rev01.patch: add stream trace macro invocations in files under 'imap' directory
20 U add-and-fix-dump-messages-to-local-misc.patch:
In the numbering above, 4-14 are essential.
But if someone tries to analyze what is going on regarding the file stream operations,
the debug dumps in 14-20 are essential.
They are part of the latest job submission.
https://treeherder.mozilla.org/jobs?repo=try-comm-central&revision=622660c032ef4334223ac3a5baa750973128e6b4
The errors I have noticed until yesterday are all intermittent category. I have weeded out permanent
ones and I verified the error recovery operation using a simulated I/O error (mostly write error) by
disconnecting net cable to the remote CIFS server where POP3 local messages are stored.
I found out that the error handling by safe I/O stream HAD TO BE DISABLED for the crude error recovery to work reasonably well. (I will write more on the next comment.)
That disabling is done in "14 A new-1242030-disable-safe-stream-base-search-local.patch".
I am uploading these to show the patch efforts are kicking and alive also to report
the error recovery does not work with the unmodified C-C very well.
BTW, I have done the initial performance check and they show the patch as expected.:
the number of write system calls was reduced, and write performance (copying/moving messages to a different folder) is faster.
As a matter of fact, I noticed xpcshell test executed sequentially (--sequential) now is much faster: it used to take about 1200 seconds on my PC, but now it is 900+ seconds. I think the 4GB folder copying operation and others suffer most from the unbufferred message write operation. But more detailed figures later.
Assignee | ||
Updated•2 years ago
|
Assignee | ||
Comment 272•2 years ago
|
||
Assignee | ||
Comment 273•2 years ago
|
||
Assignee | ||
Comment 274•2 years ago
|
||
I had to make sure that the removal of seek/tell method from a data type (bug 1719996) is OK with the buffered write approach.
Assignee | ||
Comment 275•2 years ago
|
||
Assignee | ||
Comment 276•2 years ago
|
||
Assignee | ||
Comment 277•2 years ago
|
||
Assignee | ||
Comment 278•2 years ago
|
||
Assignee | ||
Comment 279•2 years ago
|
||
Even with Ben's latest cleanup patches, there are seeks/flushes that need to be removed and this is one of the crucial removals.
(I checked the latest C-C TB and it did NOT use the buffered stream although Ben's patch DID start with buffered stream. Unnecessary seeks and flushes nullified the effect of buffering.)
Assignee | ||
Comment 280•2 years ago
|
||
After a lot of head scratching, I had to disable the use of safe stream to handle error condition.
Safe stream has been introduced by Ben's patch to hide the error(s) under the rug, so to speak.
Unfortunately, it did not work well when I tested TB's behavior.
The current C-C without my patches here
- cannot recover from simulated write error during POP3 download.
I unplugged the network cable to a CIFS server where local messages are stored.
TB shows an error dialog, but as soon as I hit the dialog, it shows another dialog, ad infinitum. This is bad. A user ought to be able to recover from such an error, at least be able to quit TB quickly to see what is causing the I/O error..
With my patch, - TB recovers from the simulated write error during POP3 download.
I unplugged the network cabe, and then TB shows the error dialog.
But as soon I hit the OK in the dialog and restores the network cable, it recovers and
waits for the next user operation (most likely the user wants to quit TB to investigate what is causing the I/O error).
I had expected the use of Safe Filestream to gets TB looking as if it were hung due to repeated
reading operation even after write error(s), but not this endlessly repeated error-dialog.
I think we may be able to insert proper check and recovery even when we use safe file stream to recover nicely. BUT, that negates the original intention of introducing safe stream and
as a matter of fact, inserting proper check and recovery are exactly what my patches try to do.
I found out the programming mistake regarding pop3 error recovery when I started to work on buffered write. (Basically it is the failure of proper error checks, and the mis-coding regarding the setting of crucial file pointer to nullptr to signal the I/O error to upper-level routine. Once these were corrected, the crude POP3 error recovery started to work and it is working with the current patch.)
Given the disabling of the use of safe stream can be done with several #if/#else/#endif and insertion of a few dozen lines of code only in a clean manner, I think we should seriously consider reverting to the legacy pop3 error recovery framework.
I don't think repeated error dialog that prohibits a user from quitting TB in the face of write error is
acceptable to general user community. Error dialog is fine, but one cannot simply quit TB cleanly.
Assignee | ||
Comment 281•2 years ago
|
||
There are a few more issues concerning error recovery.
I tested the TB's operation using the following setup.
I created a very small partition 50MB in a remote CIFS file system and
created a symlink from the ordinary message store position to this CIFS.
I could have created a different profile, but using symlink is much easier.
I tested TB under linux.
The size was small so that I can test the file system full condition during testing.
I sent 240+ e-mails to a mail test account and checked the operation.
What I found so far.:
-
Filesystem full condition was properly caught.
-
Simulated write error during pop3 message downloading by removing the remote CIFS server's network cable caused
unpatched C-C TB to enter endlessly shown error dialog loop.
(I have to send enough e-mails so that the downloading from the local POP3 server takes time to allow me the cable-unplugging operation before all the messages are downloaded.)
Even if I restored the network cable, TB could not recover to the state
where the modal error dialog is not shown so that the user can choose the menu to finish TB (or click the X button on the window frame.)
I had to kill TB from another terminal using kill PID-of-TB.Patched C-C TB showed the error dialog and once I hit OK button in the dialog and restored the cable (<- I am not sure I needed to restore the cable back when I tested the original patch a few years ago), TB was ready to receive user input and so I could quit TB using mouse.
There are a few more issues with the current C-C TB.
- I noticed the discrepancy of error messages shown during
-- the copying of messages from Inbox to Trash, and
-- "Deletion" of messages form Inbox.
You see they are actually the same operation. Deletion is the "Moving" of messages to trash holder.
I get different messages suggesting there may be two paths, and more to the point, it seems that the checks done are not quite the same.
I need to investigate.
My current plan is to investigate the two paths mentioned and if there are indeed two paths, at least we should
implement the same level of I/O error checks in these paths.
(I would investigate both the "file system full condition" and "write error during moving operation" errors for the above operations).
I really wish there is a good harness to simulate I/O errors so that TB can be tested to make sure it shows robust behavior in the face of such errors.
Using a remote file system and a network proxy to access that remote file system may be a way to go.
I wonder if something like that can be implemented on tryserver.
Actually, under linux, it may be rather easy.
The proxy can be turned off to simulute a network error and thus causing file system read error.
(However, we cannot really co-relate the timing of I/O error with TB's internal operation in fine grained manner.
So only the brute force approach of causing I/O error randomly during
the repetition of sending e-mails to TB, copying mails from one folder to the other, deletion of e-mails, compacting folders, etc.
is the way to trigger and test error handling. This is what I did several years ago over a few weeks. Back then I did not bother to create the proxy. I manually plugged and unplugged the cable. Well, actually back then I used VM Player and used its network on/off menu.)
Words of wisdom to those who may want to try such a setup.
Set the timeout of the network file system very short. Usually they are set like 90 seconds, etc.
You have to wait for a long time before the error is returned to TB. I set CIFS time out to 15 seconds or so.
Otherwise I have to spend days simply to obtain the preliminary error recovery test result I am reporting.
For CIFS, echo_interval is the ping interval. If CIFS daemon decides two consecutive ping did not return in time,
it will begin error processing. But with 5 seconds time interval, I get 15 seconds timeout. It thought it would be 10 second timeout.
Such is life. The following is the mount command I use for mounting a CIFS server (actually a router with a function to make U
SB memory stick available as CIFS file system.)
mount.cifs "//192.168.0.201/Drive(A1)" /tmp/L-temp -o username=admin,noperm,vers=1.0,sec=ntlmv2,echo_interval=5
In any case, when the abnormal error recovery behavior is observed, the only way to observe what is going at the I/O routine level is to trace
what is happening to key file stream pointers.
This is what the following patch attachments do.
Eventually, I may want to put them behind MOZ_LOG, but during this development phase, I really have to
correlate the operation with other debug messages coming from NS_WARNING, NS_ERROR that are in the C-C code already.
So I need to dump them using these macros or simple |fprintf(stderr, ...)|.
Assignee | ||
Comment 282•2 years ago
|
||
Add macros for tracing file streams.
It may be easy to turn them to use MOZ_LOG eventually.
For now, it uses an existing macro and stdout/stderr output.
At least, rewriting the code to use MOZ_LOG would be contained in this file.
That is the intention of introducing these macros.
Assignee | ||
Comment 283•2 years ago
|
||
adding calls to stream trace macro in Pop3-related files in local/src.
Assignee | ||
Comment 284•2 years ago
|
||
adding stream trace macros in files under non-pop3 files in local/src.
Assignee | ||
Comment 285•2 years ago
|
||
Assignee | ||
Comment 286•2 years ago
|
||
Assignee | ||
Comment 287•2 years ago
|
||
I wanted to clean up this pot pourri of dump messages, but figured it would be more important
to raise the issue of non-working error recovery of the current C-C TB.
Fixing the dump message can wait until we figure out what to do with the
failing error recovery.
Comment 288•2 years ago
|
||
Assignee | ||
Comment 289•2 years ago
|
||
(In reply to Ben Campbell from comment #288)
Comment on attachment 9279704 [details] [diff] [review]
new-1242030-DONT-USE-REUSABLE-new-part-1-rev04-with-the-original-uuid.patch
Thank you for the comment.
Review of attachment 9279704 [details] [diff] [review]:
I think we should start trying to land some of these.
This one is a good one to start with, to removeaReusable
from
nsIMsgPluggableStore.getNewOutputStream().My suggestions:
- break out the debug printfs into a separate patch (for developer use
rather than to land).
That is what I intend to do.
Either remove them into separate patch as I did for the later stream tracing macro calls, or
maybe hide them behind MOZ_LOG and remove #if/#endif altogether.
- This patch doesn't update the mbox and maildir GetNewOutputStream()
implementations? I assume there's another patch which does that? In any
case, those changes would need to be rolled into this patch.
I might have missed that. Let me investigate.
- I think that once getNewOutputStream() is called, it should be a concrete
requirement that eitherfinishNewMessage()
ordiscardNewMessage()
should
be called with the returned stream. This is the only way I can see the
messageStore being able to clean up properly (either after a success or an
error). This implies that passing a null stream tofinishNewMessage()
or
discardNewMessage()
should be considered an error.
That is a good point.
Problem is this.
-
Old error handling strategy adopted during pop3 download was to set the file stream to nullptr to signify an error was detected.
This is how the overall pop3 download handler kept track of the error situation in C++ code.
My patch followed this practice.I realized there is a code to truncate the half-cooked message portion that was attached to the end of a folder (presumably Inbox?).
That won't work to be sure.However, I had to disable the newer error handling using safe-stream because it did not work well with the real-world error situation.
C-C withtout my patch repeated showed error dialog and I could not quit TB cleanly. (See comment 281).
I think there may be a statement to break out of the normal processing when an error occurs.And as a matter of fact that disabling safe-stream was the only way to make the old error processing scheme to work.
That is the patch
new-1242030-disable-safe-stream-base-search-local.patch
https://bugzilla.mozilla.org/attachment.cgi?id=9279713
We may have to investigate why unpatched C-C TB loops with endless error dialog display even when the transient error caused by the
unplugging of network cable to the remote CIFS server where the message is stored clears up (the cable is plugged again.).
But I do recognize that
- removal of reuseflag, and
- desirable error handling is a separate issue.
Let me think about how to extract the removal of |reuseflag| from the patch set, and if possible, create a patch set for purely removal of reuseflag.
Yeah, in that case, probably I would move the error checks to later patch set(s).
Also, I'd recommend using phabricator to handle patches, if you're up for
it. It's now the main tool we use and it seems pretty good at handling WIP
patches which are still changing, and stacks of dependent patches. It's not
critical - the old attach-the-patch-to-the-bug method still works, but you
might find phab makes it easier.
This is what was suggested during a bugday discussion by Thomas D, and Wayne.
Maybe I should ask for a phabricator initiation session or something like that with all the policy issues clearly explained.
I tried to use phabricator when it was introduced several years ago
because it was announced to be the way to go but it was a premature announcement.
Not all the details regarding some setup that needed to be followed based on the policy decisions by administrators were in place and I did something out of line with the would be practice and so I decided to stay away from it thinking "for a while". I found out later that internal mozilla introdcutory seminars were held later, presumably to fill in those missing information regarding policy issues.
Since TB, I mean C-C developement, did not adopt phabricator immediately, that "for a while" continued until now. :-(
I am a carried-over from the old era when HG MQ extension was recommended, and so I need to switch to newer HG command practice to begin with before attempting to use Phabricator? (Or HG MQ style patch submission still work. But I digress. This probably needs separate talk.)
::: mailnews/base/public/nsIMsgPluggableStore.idl
@@ +129,5 @@* a duplicate. This gives the berkeley mailbox store a chance to simply * truncate the Inbox w/o leaving a deleted message in the store. *
- discardNewMessage closes aOutputStream always unless the passed stream
- is nullptr due to error processing..
Should we really allow a null aOutputStream to be passed in?
If something goes wrong, the mailStore might need to do some cleanup (eg
truncate the mbox file back to how it was, or delete temporary files or
whatever). And at the moment this is the only way for the mailstore to be
notified of a failure.
So obviously, in your new scheme, unlike in the legacy pop3 code, setting m_outputstream or some such to nullptr is not the way to signal that the error occurred somewhere in the low-level I/O system calls.
(Maybe we really should have an explicit m_errorflag or something that signifies an error has occurred.)
My temporal patchset here had to disable safestream-based error recovery, but as long as the filestream is non-null the
truncation happens, but I think there WERE cases when the stream was set to nullptr.
(Well, I am afraid have to simulate the error cases a few dozen times to cause the simulated error to occur an appropriate time to
cause a particular error situation where m_outputstream to nullptr and TB calls DiscardMessage. I thought I did this over a three weeks period, but as it turned out I had to do this over three months period. Error simulation to trigger various error modes to check the code took time. :-(
Unless I build a better error simulation mechanism that is tied to the timing/phase of TB processing, the timing of simulated error is pretty much random and cannot cause a desired situation to trigger a particular error processing deterministically.
We need to discuss this error handling situation further because the user is a bit perplexed unless C-C TB stops showing error dialog forever.
There is something wrong with the error recovery path, but I have yet to figure out where and how to avoid it.
(Thus, I simply adopted the old error recovery method. Well, to be exact, the code did not seem to work without disabling it with my patch set.)
@@ +130,5 @@
* truncate the Inbox w/o leaving a deleted message in the store. *
- discardNewMessage closes aOutputStream always unless the passed stream
- is nullptr due to error processing..
- (Clarification/Rationale in Bug 1121842, 1122698, 1242030)
You don't need the bug numbers here (If you really want to leave a trail
maybe put them in the commit message instead?).
I will take them out.
::: mailnews/base/src/nsMsgDBFolder.cpp
@@ +760,5 @@
static_cast<uint32_t>(rv));
+#endif
- // Return early: If we could not find an offline stream, clear the offline
- // flag which will fall back to reading the message from the server.
- if (mDatabase) mDatabase->MarkOffline(msgKey, false, nullptr);
I totally want to kill this MarkOffline() this far into the stack. It should
be handled back out by IMAP/news code which can deal with the error
properly. (related to Bug 1681487 et al)
But yeah. For now this is the same as the other failure paths, so needs to
be kept in :-)
Oh, beauty with legacy code. I will study 1681487 et al.
@@ +1721,5 @@
mDatabase->MarkOffline(messageKey, false, nullptr); // we should truncate the offline store at messageOffset ReleaseSemaphore(static_cast<nsIMsgFolder*>(this));
if (msgStore) {
// DiscardNewMessage now closes the stream all the time.
Document the current behaviour rather than history, eg:
// DiscardNewMessage() will close the stream.
@@ +1747,5 @@
- } // if (seekable)
- rv1 = rv2 = NS_OK;
- if (msgStore) {
- // FinishNewMessage now closes the stream ALL THE TIME.
Again, don't need to document the history, eg:
// FinishNewMessage() will close the stream.
Once there are enough eyeballs with the innards and the history, i will take them out.
But this close or non-close was really a source of confusion coupled with reuseflag and I had to leave it in to
keep myself aware of the issues.
Thank you again for your comments, Ben.
I think I will try to
- separate the "removal of reuseflag", and better I/O error checks necessitated by the introduction of buffered write (the error tends to be detected later due to the buffered write, and so Flush() and Close() need to be checked for errors, and actually there are so many places where error is not handled at all for Write(). It is pathetic.)
- separate the debug dump or hide them behind MOZ_LOG and remove #if/#endif,
So my intention would be producing a series of patch sets as follows.
- removal of reuse flag,
- buffered write introduction with added error checks (but how to signal the presence of an error is a big question. The legacy pop3 code sets m_outputstream or some such to nullptr for this purpose. We may have to introduce an explicit m_errorflag or something. This requires some discussion. The solution to the looping of error dialog may be incorporated here or probably come as a separate patch.)
- debug dump for developers.
- enabling of buffered write
(- disabling of the use of safe stream: could be removed depending how well the error handler is improved, but come to think of it, there ARE legitimate error cases that we can no longer write to the file system like the complete network error to remote CIFS server. So we must bear in mind the limit of what we can do. Our error recovery won't be perfect. My point is to let users quit TB cleanly in a manageable manner. That is what one needs most when we realize there is an I/O error and runaway execution of TB in such a case may tarnish the data worse. Seeing TB refusing to stop execution is a consternation in such a situation. :-(
...
Thank you again for your comment.
PS: s for phabricator, maybe I consult Wayne since he mentioned an introduction to phabricator session may be made available. :-)
Comment 290•2 years ago
|
||
(In reply to ISHIKAWA, Chiaki from comment #289)
- Old error handling strategy adopted during pop3 download was to set the file stream to nullptr to signify an error was detected.
This is how the overall pop3 download handler kept track of the error situation in C++ code.
My patch followed this practice.
Ahh... I see.
I think that's the first thing to fix then!
I propose we open another bug to change pop3 error handling to use a separate variable to signify errors rather than just nulling out the filestream. And also to make sure it calls one of FinishNewMessage()
/DiscardNewMessage()
depending on outcome.
I'll leave the NI open on this one to remind me to look at the pop3 code on Monday and write up the bug, but feel free to open a bug yourself if you'd prefer :-)
After that, I think it'll be easier to reduce your patches down into nice little atomic chunks we can start landing.
I am a carried-over from the old era when HG MQ extension was recommended, and so I need to switch to newer HG command practice to begin with before attempting to use Phabricator?
(not to get bogged down in too many details) The moz-phab commandline tool just submits changesets in Hg, wherever they came from. I'd guess it'd happily pick up any patch you applied from MQ (I don't use MQ - I just keep all my local patches in anonymous branches off my tree. I use hg rebase
to keep them up to date after pulls, and hg histedit
to chop and change and combine them ready for landing).
Assignee | ||
Comment 291•2 years ago
|
||
(In reply to Ben Campbell from comment #290)
(In reply to ISHIKAWA, Chiaki from comment #289)
- Old error handling strategy adopted during pop3 download was to set the file stream to nullptr to signify an error was detected.
This is how the overall pop3 download handler kept track of the error situation in C++ code.
My patch followed this practice.Ahh... I see.
I think that's the first thing to fix then!
I propose we open another bug to change pop3 error handling to use a separate variable to signify errors rather than just nulling out the filestream. And also to make sure it calls one ofFinishNewMessage()
/DiscardNewMessage()
depending on outcome.
I'll leave the NI open on this one to remind me to look at the pop3 code on Monday and write up the bug, but feel free to open a bug yourself if you'd prefer :-)After that, I think it'll be easier to reduce your patches down into nice little atomic chunks we can start landing.
Thank you for you comment.
OK, let me see how this goes.
Locally, I have already separated the removal of reusable flag.
Worked fine locally. So I submitted a try-comm-central job.
See
https://treeherder.mozilla.org/jobs?repo=try-comm-central&revision=1f00a8ed748893fe0f4aecaa229dfd28ae04e78e
So the next step is to introduce something like |bool m_errorflag|. Let me see how this will improve the situation.
BTW, my latest uploaded patches was based on C-C tree about a couple of weeks ago, and
it did not handle |SyncCopyStream| change and others.
It has been updated against the latest C-C as of yesterday.
Locally, it seems to work.
I am a carried-over from the old era when HG MQ extension was recommended, and so I need to switch to newer HG command practice to begin with before attempting to use Phabricator?
(not to get bogged down in too many details) The moz-phab commandline tool just submits changesets in Hg, wherever they came from. I'd guess it'd happily pick up any patch you applied from MQ (I don't use MQ - I just keep all my local patches in anonymous branches off my tree. I use
hg rebase
to keep them up to date after pulls, andhg histedit
to chop and change and combine them ready for landing).
I think I will pick up my work flow. It seems we can name patches via bookmark. Again, I will see what I can.
There are many blogs or tutorials which are definitely OUT OF DATE. This is not good for a newbie.
TIA
Assignee | ||
Comment 292•2 years ago
|
||
(I made a serious typo of writing |m_outputstream| instead of the correct |m_outFileStream|. So I corrected this comment.)|
I filed https://bugzilla.mozilla.org/show_bug.cgi?id=1773787
that takes care of the "remove reusable flag" issue first.
In the comment there, I wrote
The current plan I discussed with Ben is
1 land this "remove reusable flag" patch set.
2 Change the POP3 error handling code to use a separate EXPLICIT |m_error| flag instead of setting |m_outFileStream| to nullptr to signal an error occurred to the upper layer functions.
3 Then modify the code to properly enable buffered write.
Part-1 is here.
Part-2, -3 will be handled in bug 1242030 or maybe part-2 is handled in still another bugzilla entry.
One point. Changing the pop3 error handling code to use |m_error| flag is fine.
However, please note that setting |m_outFileStream| to nullptr gives the chance to upper layer functions to stop reading/writing data any more once there has been a fatal write error, say, and this make it possible for TB to quit downloading (or for that matter message copying operation in relevant places). This DOES make a difference for user experience when an error occurs.
I found the patch here makes TB return immediately after an error dialog popup appears so that I can decide what to do. I think many users would quit TB immediately if they realize file back end shows error behavior.
The stock C-C TB without the change somehow got into a repeated error dialog display and did not give me a chance to terminate TB cleanly.
So, I would try to disable read/write once |m_error| flag is raised as much as possible to mimic the early quit path that was taken when the |m_outFileStream| was set to nullptr.
Assignee | ||
Comment 293•2 years ago
|
||
Aha, it is |m_outFilesStream| and not |m_outputStream|. A serious typo.
So I fixed the previous comment.
Assignee | ||
Comment 294•2 years ago
|
||
Turns out
"2 Change the POP3 error handling code to use a separate EXPLICIT |m_error| flag instead of setting |m_outFileStream| to nullptr to signal an error occurred to the upper layer functions."
is relatively easy.
I can't recall the code of several years ago, but for the last few years, I only set "m_outFileStream" to nullptr when major operation occurs, basically when the final close is near.
So the change is about a few dozen lines of code.
Well, I decided to return from WriteLineToMailbox in nsPop3Sink.cpp immediately if m_error contains an error code. (I forgot to mention that I decided to store the last seen error code in m_error.)
Because it is not meaningful to continue writing if we have a write error once.
I think more work is necessary to modify code to clean up the dump statements for debugging purposes by using MOZ_LOG and other macro so that they are less obtrusive.
These should come after the patches in bug 1773787.
Description
•