690633 - HTTP blocks socket transport thread on file I/O when doing file/Blob uploads

Status: NEW

(Core :: Networking: HTTP, defect, P3)

Description

[Jason Duell]

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

[Honza Bambas (:mayhemer)]

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.

Comment 2

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

My patch in Bug 1329089 will make this issue less severe before this is fixed.

Comment 3

[Firefox Bug Husbandry Bot]

Bulk change to priority: https://bugzilla.mozilla.org/show_bug.cgi?id=1399258

Comment 4

[Firefox Bug Husbandry Bot]

Bulk change to priority: https://bugzilla.mozilla.org/show_bug.cgi?id=1399258