Closed Bug 1397635 Opened 2 years ago Closed 2 years ago

Implement PartiallySeekableInputStream to make a non seekable stream to work with http redirect

Categories

(Core :: Networking: HTTP, enhancement)

enhancement
Not set

Tracking

()

RESOLVED FIXED
mozilla57
Tracking Status
firefox57 --- fixed

People

(Reporter: baku, Assigned: baku)

References

Details

Attachments

(1 file)

In order to support nsIInputStream, non seekable in HTTP connections, we need to make HTTP redirects to work correctly: when a connection happens, a chunk of the stream is read and sent to the network. But if the http connection is redirected, necko tries to do a seek(0) in order to rewind the stream and send it again.

In this bug I'm implementing a new class called PartiallySeekableInputStream that makes any nsIInputStream, seekable for the first chunk of data. By default 4096 bytes.
jduell, can you help me to find somebody able to review this code? I discussed this issue with mayhemer but he is not accepting reviews atm.
This bug blocks a qf1 bug.
Attachment #8905383 - Flags: review?(jduell.mcbugs)
Comment on attachment 8905383 [details] [diff] [review]
partialStream.patch

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

I found this patch very straight-forward and easy to read. My compliments on a well-written piece of code. +1

I'm curious on the default buffer size of 4096 bytes. Is that set based on some heuristics or is just a finger in the air and adjust later based on logs from users?
Attachment #8905383 - Flags: review?(jduell.mcbugs) → review+
> I'm curious on the default buffer size of 4096 bytes. Is that set based on
> some heuristics or is just a finger in the air and adjust later based on
> logs from users?

Reading netwerk/protocol/http/ code it seems that 4096 is often use as default buffer for bufferedStreams, streamStorage and so on. But yes, we can adjust the value easily if needed.
Comment on attachment 8905383 [details] [diff] [review]
partialStream.patch

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

::: netwerk/base/PartiallySeekableInputStream.cpp
@@ +1,1 @@
> +/* -*- Mode: C++; tab-width: 4; indent-tabs-mode: nil; c-basic-offset: 4 -*- */

Shouldn't this 2 space indent for both new files?
/* -*- Mode: C++; tab-width: 2; indent-tabs-mode: nil; c-basic-offset: 2 -*- */
Pushed by amarchesini@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/c08a59cb75a3
Support for non-seekable stream in HTTP connection, r=bagder
https://hg.mozilla.org/mozilla-central/rev/c08a59cb75a3
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
Blocks: 1398733
Depends on: 1399215
You need to log in before you can comment on or make changes to this bug.