Closed Bug 711877 Opened 13 years ago Closed 8 years ago

nsHttpTransaction::Init defeats DEFER_OPEN optimization for file streams by calling Available on the main thread

Categories

(Core :: Networking: HTTP, defect)

defect
Not set
normal

Tracking

()

RESOLVED WONTFIX

People

(Reporter: khuey, Assigned: khuey)

References

Details

(Whiteboard: [Snappy:p2][necko-backlog])

Attachments

(1 file)

Attached patch Potential PatchSplinter Review
It looks like it should be pretty easy to defer this call to the socket thread.
Attachment #582709 - Flags: review?(mcmanus)
Comment on attachment 582709 [details] [diff] [review]
Potential Patch

Review of attachment 582709 [details] [diff] [review]:
-----------------------------------------------------------------

::: netwerk/protocol/http/nsHttpTransaction.cpp
@@ -290,5 @@
>          mRequestStream = headers;
>  
> -    rv = mRequestStream->Available(&mRequestSize);
> -    if (NS_FAILED(rv)) return rv;
> -

I would leave this code in place for the else branch of the above test (the one where there isn't a message body).. see below.

@@ +388,5 @@
> +        // If this call fails there's not much we can do.
> +        // XXXkhuey should we set mRequestSize to 0xFFFFFFFFFFFFFFFF like
> +        // nsITransportEventSink's IDL says we should?
> +        mRequestStream->Available(&mRequestSize);
> +    }

This isn't going to work in OnTransportStatus - there isn't a guaranteed event at the transaction level that happens  before data is consumed out of mrequeststream.

(resolving, connect, etc will not happen if this is a persistent connection).

but I think you can put this code in nsHttpTransaction::ReadSegments() which is called to pull data out of mRequestStream. That will capture the message body case, and you can leave the non-message body case in init because it doesn't do any I/O.

@@ +458,5 @@
>  PRUint32
>  nsHttpTransaction::Available()
>  {
> +    NS_ABORT_IF_FALSE(!NS_IsMainThread(), "Wrong thread");
> +

the necko convention I am trying to move towards:
    NS_ABORT_IF_FALSE(PR_GetCurrentThread() == gSocketThread, "wrong thread");
Attachment #582709 - Flags: review?(mcmanus)
> the necko convention I am trying to move towards:
>    NS_ABORT_IF_FALSE(PR_GetCurrentThread() == gSocketThread, "wrong thread");

Sounds good--perhaps we should add a utility function/macro for this (which can have a better error message, yet stay under 80 chars)
Kyle, can you snappy:prioritize this? It's not clear how problematic this is.
ping khuey ^
This is limited to uploads only, I think so it's fairly low priority.
Whiteboard: [Snappy] → [Snappy:p2]
Whiteboard: [Snappy:p2] → [Snappy:p2][necko-backlog]
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → WONTFIX
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: