22103 - OpenInputStream implementations need to permit same thread reading.

Status: VERIFIED FIXED

(Core :: Networking, defect, P3)

Description

[Judson Valeski]

Current OpenInputStream implementations use AsyncRead underneath which requires
the returned stream to be read on a thread other than the one which called
OpenInputStream.

Comment 1

[Brendan Eich [:brendan]]

Any thoughts on how to fix this?  Maybe use a helper thread under the covers?

/be

Comment 2

[Judson Valeski]

nice idea! Yea, Open*Stream() could spin a new thread and do basically the exact 
same thing (use SyncStreamListener()) that it does today, except that it would 
be done on the helper thread. This helper thread would be able to handle the 
incoming events from the transports, and the calling thread could block (as 
desired) until data became available via the threadsafe pipe.

Our thread usage would obviously grow which would suck, but this would solve our 
overall problem.

Comment 3

[Judson Valeski]

notes:
Here's what I'm thinking.
new class nsSyncThread which is a nsIRunnable and nsIThread

nsSyncThread::Init(nsIChannel *channel, nsIStreamListener *listener) {
 mListener = listener;
 mChannel = channel;
}

nsSyncThread::Run() {
 // call async read on this thread so 
 // events are posted here instead of the ui thread.
 mChannel->AsyncRead(mListener, ...);
 while(running) {
  event = waitForEvent();
  handleEvent(event);
 }

}

httpchannel::OpenInputStream(nsIInputStream**_retval) {
 nsIStreamListener *listener; 
 NS_NewSyncStreamListener(_retval, nsnull, &listener);
...

 nsSyncThread *syncher = new nsSyncThread();
 syncher->Init(this, listener);

 newThread(&thread);

 thread->Dispatch(syncher);
 return rv;  
}

Comment 4

[Judson Valeski]

Attachment: proposed helper class

Comment 5

[Judson Valeski]

Attachment: proposed helper class impl

Comment 6

[Judson Valeski]

Attachment: http channel diffs to use the new thread.

Comment 7

[Judson Valeski]

I've attatched the helper implementation and how it should be used in HTTP. you
have to add the helper file to the make file so it's built. I haven't tested
these though they "should" work.

Comment 8

[Brendan Eich [:brendan]]

Cc'ing pete so he can try the patches out too.

/be

Comment 9

[Brendan Eich [:brendan]]

And cc'ing [e-mail], who wants this for synchronous xml-rpc calling.

/be

Comment 10

[Stephen P. Morse]

This bug is most likely the cause of bugs 28466 and 33542.  Because of that, I 
am nominating this for nsbeta2.

Comment 11

[Judson Valeski]

I was afraid this was going to happen. Can I retract the patches :-). If these 
patches work (the http channel part probably needs to be whacked a bit), they do 
not give sync reading carte blanc to the world. Syncronous reading on the UI 
thread is VERY BAD because it halts event processing and UI transitioning.

cc'ing ruslan in hopes that he can take this bug over :-)

Comment 12

[Martijn Pieters]

Bad or not, currently peoply using sync calls don't even get to find out it is
bad for the ui thread. Things just hang for ever. The API to do sync calls is
there, being used, and broken, also for calls from non-ui thread.

Comment 13

[ruslan]

CC-ing gagan since I'll be out of town for 4 days. Yes. Unfortunately http 
handler is designed so it won't allow correct sync operations on the same 
thread. So, Jud - do you want these patches applied?

Comment 14

[Judson Valeski]

yea, if someone could run w/ these patches that would be great.

Comment 15

[ruslan]

I'm starting to look into the wallet problem and I has nothing to do with http 
handler not being able to do synchronous operations. The wallet thread actually 
creates it's own event queue and does AsyncRead. So these 2 bugs are unrelated 
the best I can tell.

Comment 16

[leger]

Putting on [nsbeta2+] radar for beta2 fix. Assigning to ruslan per valeski.

Comment 17

[ruslan]

I vote not to fix this for this release. The bug by itself doesn't block 
anything (dependencies listed are incorrect here). The synchronous client can 
implement it manually (following wallet's example). Doing it under the cover by 
creating the additional thread in the handler is not a clean solution. We'll 
address this in necko2.0 by providing trully sycnhronous way of doing this.
Gagan, 
your decision?

Comment 18

[Martijn Pieters]

From JS I can't create a seperate thread. I can't work around this bug.

Comment 19

[ruslan]

Ok. Though even as I believe that it's not critical for Beta as it won't fix any 
real issues anyway - I'm still going to fix this.

Comment 20

[ruslan]

Fixed now along the lines of the original Jud's patch

Comment 21

[Tom Everingham]

verified:
winNT 2000070508

Comment 22

[Pulsebot]

Pushed by ffxbld-merge:
https://hg.mozilla.org/releases/mozilla-beta/rev/6983e3e65a4c
[fenix] Close https://github.com/mozilla-mobile/fenix/issues/22103: Add recent bookmarks telemetry