Closed
Bug 22103
Opened 25 years ago
Closed 24 years ago
OpenInputStream implementations need to permit same thread reading.
Categories
(Core :: Networking, defect, P3)
Tracking
()
VERIFIED
FIXED
M18
People
(Reporter: jud, Assigned: ruslan)
Details
(Whiteboard: [nsbeta2+],2d)
Attachments
(3 files)
(deleted),
text/plain
|
Details | |
(deleted),
text/plain
|
Details | |
(deleted),
patch
|
Details | Diff | Splinter Review |
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•25 years ago
|
Target Milestone: M13
Reporter | ||
Updated•25 years ago
|
Severity: normal → enhancement
Target Milestone: M13 → M18
Comment 1•25 years ago
|
||
Any thoughts on how to fix this? Maybe use a helper thread under the covers?
/be
Reporter | ||
Comment 2•25 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•25 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•25 years ago
|
||
Reporter | ||
Comment 5•25 years ago
|
||
Reporter | ||
Comment 6•25 years ago
|
||
Reporter | ||
Comment 7•25 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.
Comment 8•25 years ago
|
||
Cc'ing pete so he can try the patches out too.
/be
Comment 9•25 years ago
|
||
And cc'ing mj@digicool.com, who wants this for synchronous xml-rpc calling.
/be
Comment 10•25 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
Reporter | ||
Comment 11•25 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•25 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•25 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•25 years ago
|
||
yea, if someone could run w/ these patches that would be great.
Assignee | ||
Comment 15•25 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•25 years ago
|
||
Putting on [nsbeta2+] radar for beta2 fix. Assigning to ruslan per valeski.
Assignee: valeski → ruslan
Whiteboard: [nsbeta2+]
Assignee | ||
Comment 17•25 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?
Comment 18•25 years ago
|
||
From JS I can't create a seperate thread. I can't work around this bug.
Assignee | ||
Comment 19•24 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•24 years ago
|
||
Fixed now along the lines of the original Jud's patch
Status: ASSIGNED → RESOLVED
Closed: 24 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•