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)
Tracking
()
VERIFIED
FIXED
People
(Reporter: dougt, Assigned: dougt)
Details
Attachments
(3 files)
1.24 KB,
patch
|
Details | Diff | Splinter Review | |
3.18 KB,
patch
|
Details | Diff | Splinter Review | |
3.26 KB,
patch
|
Details | Diff | Splinter Review |
When the socket transport thread goes away, it should cancel all active
transports. If it does not, they will leak.
Assignee | ||
Comment 1•24 years ago
|
||
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.
Assignee | ||
Comment 2•24 years ago
|
||
Assignee | ||
Comment 3•24 years ago
|
||
Assignee | ||
Comment 4•24 years ago
|
||
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.
Comment 5•24 years ago
|
||
r=waterson for xpcom patch.
Assignee | ||
Comment 6•24 years ago
|
||
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.
Assignee | ||
Comment 7•24 years ago
|
||
Comment 8•24 years ago
|
||
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
Assignee | ||
Comment 9•24 years ago
|
||
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.
Comment 10•24 years ago
|
||
r=darin for the nsSocketTransportService patch.
Comment 11•24 years ago
|
||
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
Assignee | ||
Comment 12•24 years ago
|
||
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.
Assignee | ||
Comment 13•24 years ago
|
||
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
You need to log in
before you can comment on or make changes to this bug.
Description
•