Closed Bug 470221 Opened 11 years ago Closed 11 years ago

imap autosync leaves db's open

Categories

(MailNews Core :: Backend, defect, P3)

x86
Windows XP
defect

Tracking

(Not tracked)

RESOLVED FIXED
Thunderbird 3.0b3

People

(Reporter: Bienvenu, Assigned: Bienvenu)

References

Details

(Whiteboard: [has patch for review])

Attachments

(2 files, 1 obsolete file)

Attached patch proposed fix (obsolete) — Splinter Review
In some situations, IMAP autosync leaves the .msf files for imap folders open, because the folder caches db's.  This can cause large memory bloat if you have large folders, and consumes file handles, which are a limited resource, especially on some systems.

We need a more general solution for this that doesn't rely on the db/folder consumers, but for now, this keeps memory usage under control for me...

I believe autosync is generally done with the folder at the places where I've made it tell the folder to release its cached db pointer, but worst case, we'll just re-open the db.
Attachment #353679 - Flags: superreview?(bugzilla)
Attachment #353679 - Flags: review?(bugmil.ebirol)
For message downloads, currently we close the folders in nsAutoSyncState::SetState method based on the following conditions;

if (!folderOpen && ! (folderFlags & nsMsgFolderFlags::Inbox))
  ownerFolder->SetMsgDatabase(nsnull);

Since it is guaranteed that we set the state of the folders to stCompletedIdle before we remove it from the queue, I don't think the leak stems from this part of the code.

On the other hand it looks like we forget to close the database explicitly when we update the folders, and my guess is this is the source of the problem.

My suggestion is to keep close in nsAutoSyncState::ProcessExistingHeaders method;

     mExistingHeadersQ.Clear();
     mProcessPointer = 0;
+    folder->SetMsgDatabase(nsnull);

but not introduce new database close operations to the download part since it contradicts with the final close, and closing the database every time we download a group of messages come with a price. If we really want to close the database of the pending folders during non-idle time, we can do that in the autosync manager when we get an idle-back signal from the idle service.
ok, thx, I'll try just that one change and get back to you...
ok, let's just try the part that Emre wants, since it should help and this bug is causing a fair amount of pain. We can deal with any remaining issues as they come up.
Attachment #353679 - Attachment is obsolete: true
Attachment #354302 - Flags: superreview?(bugzilla)
Attachment #354302 - Flags: review?(bugmil.ebirol)
Attachment #353679 - Flags: superreview?(bugzilla)
Attachment #353679 - Flags: review?(bugmil.ebirol)
Attachment #354302 - Flags: review?(bugmil.ebirol) → review+
Attachment #354302 - Flags: superreview?(bugzilla) → superreview+
fix checked in.
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
this fix is not sufficient - I believe we need at least one of the other changes in my previous patch. I'm running with my patch and after a while, auto sync has all my imap folder db's open. Perhaps they're all waiting to be synced in a queue, it's not good. but Re-opening.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
In this case, I would suggest to close dbs of the queued folders when idle is back.
should this be blocking or wanted?
I'm going to mark this blocking, yes.
Flags: blocking-thunderbird3+
Target Milestone: --- → Thunderbird 3.0b3
Whiteboard: [needs extra patch]
Blocks: 462539
Priority: -- → P3
If we do a status on an imap folder and decide we don't need to update it, then we should let go of the cached folder db...
Attachment #376981 - Flags: superreview?(bugzilla)
Attachment #376981 - Flags: review?(bugzilla)
Status: REOPENED → ASSIGNED
Whiteboard: [needs extra patch] → [has patch for review]
Comment on attachment 376981 [details] [diff] [review]
let go of folder's cached db if status says there are no new headers

Yep that's much tidier. r/sr=Standard8
Attachment #376981 - Flags: superreview?(bugzilla)
Attachment #376981 - Flags: superreview+
Attachment #376981 - Flags: review?(bugzilla)
Attachment #376981 - Flags: review+
fix checked in.
Status: ASSIGNED → RESOLVED
Closed: 11 years ago11 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.