Open
Bug 690633
Opened 13 years ago
Updated 2 years ago
HTTP blocks socket transport thread on file I/O when doing file/Blob uploads
Categories
(Core :: Networking: HTTP, defect, P3)
Core
Networking: HTTP
Tracking
()
NEW
People
(Reporter: jduell.mcbugs, Unassigned)
References
Details
(Whiteboard: [necko-backlog])
In search of enlightenment on how to properly upload a file from the filesystem onto a socket (I need to do this for Blob support in Websockets), I looked at how our HTTP code does it. It turns out we block the entire socket transport thread on file I/O whenever we do HTTP file uploads (or send XHR Blobs that are backed by a file). I'm assuming that's a Bad Thing (and if it's not a big deal now for occasional file uploads, it'll be one for Web 6.0++, when the Blobs are a-flyin' freely). Here's the stack trace from hitting POST on a form with a file upload: #0 nsFileInputStream::Read at /netwerk/base/src/nsFileStreams.cpp:367 -- Calls PR_Read(), which blocks until it returns data from disk. #1 in nsBufferedInputStream::Fill at /netwerk/base/src/nsBufferedStreams.cpp:399 #2 in nsBufferedInputStream::ReadSegments at /netwerk/base/src/nsBufferedStreams.cpp:363 #3 in nsBufferedInputStream::Read at /netwerk/base/src/nsBufferedStreams.cpp:335 #4 in nsMultiplexInputStream::Read at /xpcom/io/nsMultiplexInputStream.cpp:224 #5 in nsMultiplexInputStream::Read at /xpcom/io/nsMultiplexInputStream.cpp:224 #6 in nsMIMEInputStream::Read at /netwerk/base/src/nsMIMEInputStream.cpp:282 #7 in nsMultiplexInputStream::Read at /xpcom/io/nsMultiplexInputStream.cpp:224 #8 in nsBufferedInputStream::Fill at /netwerk/base/src/nsBufferedStreams.cpp:399 #9 in nsBufferedInputStream::ReadSegments at /netwerk/base/src/nsBufferedStreams.cpp:363 #10 in nsHttpTransaction::ReadSegments at /netwerk/protocol/http/nsHttpTransaction.cpp:488 #11 in nsHttpConnection::OnSocketWritable at /netwerk/protocol/http/nsHttpConnection.cpp:709 #12 in nsHttpConnection::OnOutputStreamReady at /netwerk/protocol/http/nsHttpConnection.cpp:940 #13 in nsHttpConnection::Activate at /netwerk/protocol/http/nsHttpConnection.cpp:191 #14 in nsHttpConnectionMgr::DispatchTransaction at /netwerk/protocol/http/nsHttpConnectionMgr.cpp:866 #15 in nsHttpConnectionMgr::nsHalfOpenSocket::OnOutputStreamReady at /netwerk/protocol/http/nsHttpConnectionMgr.cpp:1577 #16 in nsSocketOutputStream::OnSocketReady at /netwerk/base/src/nsSocketTransport2.cpp:514 #17 in nsSocketTransport::OnSocketReady at /netwerk/base/src/nsSocketTransport2.cpp:1531 #18 in nsSocketTransportService::DoPollIteration at /netwerk/base/src/nsSocketTransportService2.cpp:738 #19 in nsSocketTransportService::Run at /netwerk/base/src/nsSocketTransportService2.cpp:631 It looks like we didn't intend this to happen: nsHttpTransaction::ReadSegments has code that checks to see if mRequestStream->ReadSegments() returns NS_BASE_STREAM_WOULD_BLOCK, and if so we do // if read would block then we need to AsyncWait on the request stream. // have callback occur on socket thread so we stay synchronized. if (rv == NS_BASE_STREAM_WOULD_BLOCK) { nsCOMPtr<nsIAsyncInputStream> asyncIn = do_QueryInterface(mRequestStream); if (asyncIn) { nsCOMPtr<nsIEventTarget> target; gHttpHandler->GetSocketThreadTarget(getter_AddRefs(target)); if (target) asyncIn->AsyncWait(this, 0, 0, target); But this is defeated by the fact that nothing in the big pile 'o streams returns NS_BASE_STREAM_WOULD_BLOCK. I can think of two solutions here: 1) add a new nsNonBlockingBufferedInputStream class (or add nsIAsyncInputStream support into the existing nsBufferedInputStream class), and instead of having its ReadSegments() function call Fill() when its buffer is empty (which does a blocking Read), it should return NS_BASE_STREAM_WOULD_BLOCK and arrange to fill it's buffer in a non-blocking way, presumably by sending the I/O to a reader thread. 2) We already have a class that's devoted to turning blocking streams into async ones: nsIStreamTransportService. If we determine that the upload stream contains a blocking stream, we should probably use this class instead of reading from the stream directly. However, this will at a mimumum require us to change the implementation of nsMultiplexInputStream::IsNonBlocking(). Right now it returns "true" if *any* of the streams it wraps are nonblocking. Instead it would only return true if *all* of its substreams are nonblocking (that makes a whole lot more sense to me anyway, no?). I haven't looked into what this change might break, however, and I also don't know how well nsIStreamTransportService plays with streams that are already nonblocking (there's a mix of blocking/nonblocking streams in a form POST): the IDL says the stream 'must implement "blocking" stream semantics': not clear on what that means in practice. #2 sounds better to me, but the changes for #1 sound narrower in scope. Thoughts?
Comment 1•13 years ago
|
||
I new I had seen this somewhere... nsIStreamTransportService may be a solution here. It creates an nsITransport from nsIInputStream. We can then query that transport for an input stream, that is async (one side of a pipe) that is async copied on a pooled background thread.
Updated•9 years ago
|
Whiteboard: [necko-backlog]
Comment 2•8 years ago
|
||
My patch in Bug 1329089 will make this issue less severe before this is fixed.
Comment 3•7 years ago
|
||
Bulk change to priority: https://bugzilla.mozilla.org/show_bug.cgi?id=1399258
Priority: -- → P1
Comment 4•7 years ago
|
||
Bulk change to priority: https://bugzilla.mozilla.org/show_bug.cgi?id=1399258
Priority: P1 → P3
Updated•2 years ago
|
Severity: normal → S3
You need to log in
before you can comment on or make changes to this bug.
Description
•