OpenInputStream implementations need to permit same thread reading.

VERIFIED FIXED in M18

Status

()

Core
Networking
P3
major
VERIFIED FIXED
19 years ago
18 years ago

People

(Reporter: Judson Valeski, Assigned: ruslan)

Tracking

Trunk
x86
Other
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [nsbeta2+],2d)

Attachments

(3 attachments)

(Reporter)

Description

19 years ago
Current OpenInputStream implementations use AsyncRead underneath which requires
the returned stream to be read on a thread other than the one which called
OpenInputStream.
(Reporter)

Updated

19 years ago
Target Milestone: M13
(Reporter)

Updated

19 years ago
Blocks: 11859
(Reporter)

Updated

19 years ago
Severity: normal → enhancement
Target Milestone: M13 → M18

Updated

18 years ago
Blocks: 37913
Any thoughts on how to fix this?  Maybe use a helper thread under the covers?

/be
(Reporter)

Comment 2

18 years ago
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.
(Reporter)

Comment 3

18 years ago
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;  
}
(Reporter)

Comment 4

18 years ago
Created attachment 8472 [details]
proposed helper class
(Reporter)

Comment 5

18 years ago
Created attachment 8473 [details]
proposed helper class impl
(Reporter)

Comment 6

18 years ago
Created attachment 8474 [details] [diff] [review]
http channel diffs to use the new thread.
(Reporter)

Comment 7

18 years ago
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.
Cc'ing pete so he can try the patches out too.

/be
And cc'ing mj@digicool.com, who wants this for synchronous xml-rpc calling.

/be

Comment 10

18 years ago
This bug is most likely the cause of bugs 28466 and 33542.  Because of that, I 
am nominating this for nsbeta2.
Severity: enhancement → major
Keywords: nsbeta2

Updated

18 years ago
Blocks: 33542

Updated

18 years ago
Blocks: 28466
(Reporter)

Comment 11

18 years ago
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

18 years ago
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.
(Assignee)

Comment 13

18 years ago
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?
(Reporter)

Comment 14

18 years ago
yea, if someone could run w/ these patches that would be great.
(Assignee)

Comment 15

18 years ago
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

18 years ago
Putting on [nsbeta2+] radar for beta2 fix. Assigning to ruslan per valeski.
Assignee: valeski → ruslan
Whiteboard: [nsbeta2+]
(Assignee)

Comment 17

18 years ago
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?
(Assignee)

Updated

18 years ago
No longer blocks: 11859, 28466, 33542, 37913

Comment 18

18 years ago
From JS I can't create a seperate thread. I can't work around this bug.
(Assignee)

Comment 19

18 years ago
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.
Status: NEW → ASSIGNED
Whiteboard: [nsbeta2+] → [nsbeta2+],2d
(Assignee)

Comment 20

18 years ago
Fixed now along the lines of the original Jud's patch
Status: ASSIGNED → RESOLVED
Last Resolved: 18 years ago
Resolution: --- → FIXED

Comment 21

18 years ago
verified:
winNT 2000070508
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.