carglue a t ya hoo do tcom Assignee
	Description • 15 years ago

User-Agent:       Mozilla/4.0 (compatible; MSIE 7.0; Windows NT 5.1; Trident/4.0; iOpus-I-M; iOpus-Web-Automation; Mozilla/4.0 (compatible; MSIE 6.0; Windows NT 5.1; SV1) ; .NET CLR 1.1.4322; .NET CLR 2.0.50727; .NET CLR 3.0.4506.2152; .NET CLR 3.5.30729; yie8)
Build Identifier: 

Currently there is no way to forcibly terminate existing keep-alive connections in Firefox without shutting down.  This action may be needed for extensions that require network connections to be refreshed prior to generating metrics or measurements between runs, as normally Firefox tries hard to keep connections open when asked to.

The patch provided permits terminating any dead, idle, or keep-alive connections via a simple "keep-alive-net-teardown" nofication.  This is similar to what happens during shutdown, except that none of the other shutdown listeners get triggered.

Comments welcomed.

Reproducible: Always

	Matthias Versen [:Matti]
	Comment 2 • 15 years ago

Comment on attachment 410912 [details] [diff] [review]
Patch to teardown network connections via notification

You have to request a review. As non developer I can only guess who could do that

	Kyle Huey (Exited; not receiving bugmail, old account, do not use)
	Comment 3 • 15 years ago

It appears that you've built this patch against a version of the Mozilla source you checked out from CVS.  We stopped active development on CVS quite a while ago and moved to Mercurial.  Your code should apply more or less unchanged to the current source, but the diff format is a bit different.  

https://developer.mozilla.org/En/Developer_Guide/Source_Code/Mercurial
is a good place to start if you're not familiar with it.

	Honza Bambas (:mayhemer)
	Comment 4 • 15 years ago

carglue a t ya hoo do tcom:

Please add a patch based on mozilla-central repo. Best is to use mercurial queues for this. It looks good, I could do a review and will probably give you r+. Just remove the empty line you add in nsHttpHandler::Observe.

One question: do you realize that this will also break connections (even non-keep-alive) in the middle of transfer? Do you need to break also the active connections? (I have to re-check under what conditions connections are present in the mActiveConns array).

	carglue a t ya hoo do tcom Assignee
	Comment 5 • 15 years ago

Yes, the patch was intended to break all existing connections when the special notification is recieved, identical to what is done during Firefox shutdown.  I was mostly calling attention to keep-alives because those connections tend to be 'sticky', and also because there is currently no other way from within an addon to tell Firefox to stop keeping those connections alive.  This is true regardless of whether the user presses the browser's Stop button or if the extension's code loops through all .isPending requests and calls .cancel() on each of them.

In our code, our "resetNetworkActivity" method that first cancels all existing network requests as BINDING_ABORTED, followed by disabling all http activity observation, followed by sending the "keep-alive-net-teardown" notification to necko to close any persistent server connections (keep-alives).  In this way, our other network hooks receive and process any canceled network requests first as each request is canceled, prior to our forcing any network channel closures.

I'll update the patch to go against the latest mecurial sources.

	carglue a t ya hoo do tcom Assignee
	Comment 6 • 15 years ago

patch updated to diff against latest 3.7 hg

	Kyle Huey (Exited; not receiving bugmail, old account, do not use)
	Comment 7 • 15 years ago

Comment on attachment 412008 [details] [diff] [review]
Patch to teardown network connections via notification - hg diff

resetting original review flags

	Kyle Huey (Exited; not receiving bugmail, old account, do not use)
	Comment 8 • 15 years ago

(In reply to comment #6)
> Created an attachment (id=412008) [details]
> Patch to teardown network connections via notification - hg diff
> 
> patch updated to diff against latest 3.7 hg

Thanks for the updated patch.  I've reset the review flags appropriately.

	Honza Bambas (:mayhemer)
	Comment 9 • 15 years ago

You have wrong indention in the patch.

	Jason Duell
	Comment 10 • 13 years ago

Comment on attachment 412008 [details] [diff] [review]
Patch to teardown network connections via notification - hg diff

This fell off my radar ages ago--my apologies.   Patrick is much more familiar with the current state of the connection manager code, so I'll let him decide if this is still worth doing.  Honza could review instead, but his queue is already long.

	Patrick McManus [:mcmanus]
	Comment 11 • 13 years ago

Comment on attachment 412008 [details] [diff] [review]
Patch to teardown network connections via notification - hg diff

Review of attachment 412008 [details] [diff] [review]:
-----------------------------------------------------------------

I don't think an API to rip down connections (regardless of state) is a good idea.

we already have a function to close idle connections and mark active ones for closure as soon as they complete. It's called nsHttpConnectionMgr::ClosePersistentConnections().

I would r+ a patch that called that off of net:persistent-connection-teardown

	Patrick McManus [:mcmanus]
	Comment 12 • 9 years ago

net:prune-all-connections