33824 - make necko thread safe

Status: RESOLVED WONTFIX

(Core :: Networking, defect, P3)

Description

[Gagan]

I plan to write a test case that fires multiple threads to pull down test
documents simultaneously. Bill's initial efforts seem to have hit some problems
that I hope to fix as I see them.

Comment 1

[Gagan]

->m16

Comment 2

[Warren Harris]

Moving to M17 which is now considered part of beta2.

Comment 3

[Gagan]

ruslan: this is the bug that I talked to you about. A lot of Bill Law's bugs are
dependeent on this one directly or indirectly. 

Comment 4

[ruslan]

Well. Necko is not thread safe and we can't fix it before tomorrow :-( It 
depends on the event sequencing through the event queues (which may be just one 
queue on windows I believe). Is it the bug about everything slowing down while 
files are being downloaded?

Comment 5

[Bill Law]

Yes, because I can't move the download/save to another thread, we do it on the 
UI thread.  A previous approach was to use AsyncWrite (which, as I understand 
it, has Necko move the read/write to another thread) but that started causing a 
complete hang just prior to PR1.  We went to the synchronous read/write because 
it at least didn't hang.

It might be possible to go back to the AsyncWrite code and resolve the problem 
that caused the hang.  You can revert to that version of the code (presuming it 
hasn't suffered any code-rot in the interim) by uncommenting/commenting a switch 
at the top of some file in mozilla/xpfe/components/xfer/src (I don't remember 
precisely where but it's commented thoroughly enough).  I was going to try that 
myself if and when I found the time.

Comment 6

[ruslan]

I see it still using AsyncWrite to write to the file channel, while it's using 
OpenInputStream to read from the network (AsyncRead is ifdefed). Essentially 
OpenInputStream for http does AsyncRead under the cover, so it won't make any 
difference. Is it http or ftp-specific problem? Http handler runs on the UI 
thread (with marshalling data from the socket thread), while ftp runs on it's 
own thread. We probably should be doing async reads and sync writes here, as 
xfer size between the socket thread and the handler is limited to 8K. Any 
comments?

Comment 7

[ruslan]

I'm confused now. USE_ASYNC_READ is enabled in the code. 

Comment 8

[Bill Law]

Yes, USE_ASYNC_READ means "read the input stram via AsyncRead and write the 
output synchronously using Write."  If you turn that off, it then uses 
AsyncWrite on the output stream and passes the input stream.

The latter uses multiple threads so is conceptually similar to the 
TestThreadedIO test in mozilla/netwerk/test.  But clearly AsyncWrite has solved 
some of the "thread safety" issues that are exhibited by TestThreadedIO.

BTW, I'm not sure we really need "threadsafe" Necko.  Maybe we just need to be 
able to read necko input streams on secondary threads (not just the UI thread). 
 I don't think that's the same thing.

Comment 9

[ruslan]

There's smth. else (necko-unrelated) going on there. I just completely commented 
out actual writting into the file and it's still as slow with ftp. When I click 
on links - they sometimes load, sometimes don't, reporting (error loading url). 
Looks like somebody is cancelling the load or smth.

Comment 10

[Gagan]

pulling in ruslan's necko bugs ->darin

Comment 11

[Darin Fisher]

->me

Comment 12

[Darin Fisher]

law: is this still desirable?

Comment 13

[Bill Law]

I dunno.  At the time, I was trying to load a URL on another thread so that the
UI thread would remain more responsive.  That seamed reasonable.  But I don't
understand our threading story enough to know what's reasonable and what's not.
 We do a fair amount of shifting of stuff to other threads within Necko already
so maybe that means clients shouldn't need, or try, to do it themselves.

Comment 14

[benc]

mass move, v2.
qa to me.

Comment 15

[benc]

+mozilla1.0

It might be a good milestone to make a final decision on this...

Comment 16

[Darin Fisher]

http will become much closer to being threadsafe as a result of the fix for bug
84019.

Comment 17

[Christian :Biesinger (don't email me, ping me on IRC)]

current state, according to darin:
consumers of ioservice and protocol handlers must all live on one thread.
cache, dns, and i/o transports can be used by any thread

In effect, this means that channels and uris can only be created on the main
thread, which is what this bug is about.

Comment 18

[Patrick McManus [:mcmanus]]

there are separate bugs open for dealing with threadsafe uris, which are unfortunately complicated by the fact that they can sometimes be created by javascript protocol handlers. The details in this bug have been overtaken by events