Closed Bug 63565 Opened 24 years ago Closed 24 years ago

Socket transport service does not cancel sockets before shutting down.

Categories

(Core :: Networking, defect)

x86
Windows 2000
defect
Not set
normal

Tracking

()

VERIFIED FIXED

People

(Reporter: dougt, Assigned: dougt)

Details

Attachments

(3 files)

When the socket transport thread goes away, it should cancel all active transports. If it does not, they will leak.
Two problem cause this. First, any events that are create and posted during xpcom shutdown, specifically during xpcom service shutdown, are lost. This is terrible for necko since we create async messages which AddRef various nsIChannel's. If these messages are not handled, we leak references as well as we do not complete any pending transactions. The second problem is more obvious. First we need to cancel any transport that is in our active transport list durning shutdown. In the ~() of the service, I also release anything that is still in the active list. We also have to make sure that the the socket transport posting thread does not go away by holding a reference to the event queue service. Without holding this reference, I see the event chain get whacked. One other unrelated changes was to create the string bundle via a thread proxy. This was so that if the transport service was ever created on a non-ui thread, the string bundle stuff wouldn't bitch. patches coming soon.
Attached patch xpcom patchSplinter Review
Attached patch socket patchSplinter Review
Darin, could you review the socket patch. (I didn't mess around with the ordering of suspend and resume. I will leave that for another day/bug) Chris-da-man, could you review that I did not bodyslam xpcom too hard.
r=waterson for xpcom patch.
The socket patch is not entirely correct. I should be releasing the transports during the Shutdown method in the service, not the destructor. If I do not release them in the Shutdown, we will get cycles. Darin, please review the next patch.
Attached patch socket patch 2Splinter Review
hey doug, I think that the patch looks good... The only part that I don't understand is why you need to hold onto mEventQueueService ? It looks like this is the eventQ of the UI thread... What is the point of defering the destruction of the UI eventQ until after the SocketTransportService has been destroyed? -- rick
Hey rick, thanks for your comments. the point is that during the destruction of the socket transport service, we may destory (via cancel()) active transports. (FTP caches control connections. The logic if very simple now. Hold onto the connection forever. This will change, but for now it will do... Also, note that when http pipelining is enabled (and works :-), they may cache transport in a similar fashion.) Now, when we Shutdown() the STS, any remaining transport we will cancel. They may fire async notification on to the UI thread. If we do not hold onto the event service during the the destruction of the sts, the UI event queue will be marked as not-accepting. (hence the assert without caching the event queue service). The notifications holds a references on the transport plus the event could not be posted successfully, so we will leak.
r=darin for the nsSocketTransportService patch.
Doug your socket changes look good to me. Just for kicks, have you tried them out with imap? Like ftp, we also do a lot of connection caching. Just want to make sure this doesn't rock the imap boat any. pending that, sr=mscott
thanks. no problems with imap that I could see. I sent and received mail without a problem. I would only expect shutdown problems (e.g. crash on shutdown), but with the last socket patch, I have not seen any.
fixed checked. The socket patch was missing a check for null on line 622. I added that too.
Status: NEW → RESOLVED
Closed: 24 years ago
Resolution: --- → FIXED
verified per engineer's comments
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: