Consider the following condition: 1) Code creates an nsPipe with a fixed size X. 2) The original pipe input stream is Clone()'d. 3) stream1 is read rapidly, while stream2 is not read at all The pipe code will continue to maintain the internal buffer until stream 2 is read. Since stream 2 is not reading, this means the writer will block after writing X bytes. When the writer blocks, the reader on stream1 is also blocked from continuing. While I think this is reasonable from a resource usage point of view, we could consider trying to mitigate it in a few different ways: 1) Dynamically increase the size of the pipe buffer to (original-size * number-of-input-streams). When input streams are closed the size would be reduced, although we would have to wait for reading to occur before memory could actually be free'd. 2) Increase the size to effectively infinite allowing the input streams to be read at completely independent rates. This of course runs the risk of OOM. I don't think we need to do anything about this immediately. I just wanted to note the potential issue in a bug for discussion in case it comes up.

Note, this is no different from what happens with our previous tools as well. With nsInputStreamTee data is synchronously copied to an nsIOutputStream as the first stream is read. Theoretically this output stream is the writing end of an nsPipe. When that nsPipe fills up you block, which also blocks the tee from reading. This is in addition to the problem of the case where the original tee stream is not getting read at all which immediately starves the tee'd pipe.

There is also discussion of how to handle this sort of thing in the whatwg-streams spec. https://github.com/whatwg/streams/issues/271 While we don't directly expose nsPipe as one of these streams, we are currently using our pipe semantics directly in the Fetch Request/Response Clone() implementations. In that case, though, Fetch sets an "infinite" pipe to simulate XHR's lack of back pressure. This effectively avoids the divergent reading problem for these Request/Response Clone(), but we may have to deal with it in the future for other stream APIs.

I think we need to solve this. I suggest automatically raising the size of the nsPipe to "infinite" when a Clone() occurs. Otherwise we will get unexpected behavior if two sides of the clone are read at different rates. For example, ServiceWorkerManager clones the upload stream when a POST request is intercepted. If the service worker does something as simple as: addEventListener('fetch', function(event) { event.respondWith(fetch(event.request)); }); A large POST body can end up deadlocking the network request. The SW tries to consume its half of the clone as fast as possible. The other half of the clone is never read because the default upload does not occur. The end result is that the fetch(event.request) blocks after reading the first 64kb of the upload stream. Two main options: 1) The simplest thing to do to avoid this situation is to remove the size limit when we have one or more clones attached to the same pipe. I prefer this option. 2) An alternative would be to make NS_CloneInputStream() create infinite pipes when it follows its fallback path. This does not help people who started out with an nsPipe, though, and leaves them to discover the stalling behavior themselves. Nathan, what do you think?

On second thought, I think this should be a ServiceWorkers-v1 thing. It should be relatively simple to fix and avoiding deadlocks is a good thing. I think largish uploads is something we are likely to see on real sites when we release.

After thinking about this some more I have a better solution. Making the pipe "infinite" after a clone would be bad because something like NS_AsyncCopy() would race ahead even if neither of the cloned streams is being read. This complete breakage of back pressure is undesirable. Instead, I propose changing the pipe "max size" from meaning: "nsPipe max size determines the maximum size of the underlying nsSegmentedBuffer." To: "nsPipe max size determines maximum amount of buffering permitted beyond the fastest reader." This modified definition effectively devolves into the original definition if there is only reader stream from the pipe. With cloned streams in play, the pipe can grow to "original max size + largest difference in cloned stream reader positions". So slower reader streams effectively get "infinite" buffering, but we still apply backpressure for data that hasn't been read yet. In terms of implementation, this would mean: 1) Make the nsSegmentedBuffer always "infinite" in size. 2) Modify nsPipe::GetWriteSegment() to not simply call nsSegmentedBuffer::AppendNewSegment(). 3) Instead, GetWriteSegment() should check how far ahead of the fastest reader we are and return NS_BASE_STREAM_WOULD_BLOCK if the pipe is already at the limit.

(In reply to Ben Kelly [:bkelly] from comment #5) > After thinking about this some more I have a better solution. > > Making the pipe "infinite" after a clone would be bad because something like > NS_AsyncCopy() would race ahead even if neither of the cloned streams is > being read. This complete breakage of back pressure is undesirable. > > Instead, I propose changing the pipe "max size" from meaning: > > "nsPipe max size determines the maximum size of the underlying > nsSegmentedBuffer." > > To: > > "nsPipe max size determines maximum amount of buffering permitted beyond > the fastest reader." > > This modified definition effectively devolves into the original definition > if there is only reader stream from the pipe. With cloned streams in play, > the pipe can grow to "original max size + largest difference in cloned > stream reader positions". > > So slower reader streams effectively get "infinite" buffering, but we still > apply backpressure for data that hasn't been read yet. Just to be clear, this means that we're going to buffer the entire in memory because we're still waiting for the second (or third, or fourth...) reader to start looking at the buffer? And in the case of SW, it sounds like we're never going to start with that second reader? (This is what comment 3 sounds like, and it seems unfortunate.) I guess we don't have very many options here, so going with the above solution sounds reasonable enough.

(In reply to Nathan Froyd [:froydnj] from comment #6) > Just to be clear, this means that we're going to buffer the entire in memory > because we're still waiting for the second (or third, or fourth...) reader > to start looking at the buffer? Correct. The buffer expands to encompass the spread between the fastest and slowest reader. > And in the case of SW, it sounds like we're > never going to start with that second reader? (This is what comment 3 > sounds like, and it seems unfortunate.) In the 99% case yes. But... There is the possibility for them to be read simultaneously. For example, if a service worker starts reading the upload stream asynchronously and then returns without calling respondWith() before completing the read. In that case both the SW and the original necko channel are reading both halves of the clone at the same. Incidentally, this is why we need clones and can't just use stream rewinding for service workers.

We're not going to be using cloned pipes for upload streams when interception with service workers, so this is no longer an immediate priority.

We probably need to work on this soonish. Its a compat issue for fetch .clone(). I've seen one person encounter it so far in bug 1253777 comment 9.

Another dev has reported hitting this so I'm going to work it. I have a plan already formulated, so I think it would be quickest if I just did it.

This basically implements the plan from comment 5, but it was a bit more complicated. In addition to those previously outlined steps I also had to adjust the code which signals when the pipe is no longer blocked for writing.

The existing AsyncWait() tests exercising cloned streams also needed to change. I think I will probably clean this up a bit, but it passes for now. Let's see what try says: https://treeherder.mozilla.org/#/jobs?repo=try&revision=d205899a034e7d4e344df51082b050547ed9afd8

I decided to simply incorporate the behavior in the existing test cases instead of writing a new test case. This revealed a few bugs in the P1 patch around when the AsyncWait() callback is fired and also when a divergent stream is closed.

This fixes the issues described in the previous comment. It also includes various cleanup like more comments, more const methods, etc.

Comment on attachment 8815795 [details] [diff] [review] P1 Allow pipe cloned streams to be read at different rates. r=froydnj This patch alters the maximum buffer size of nsPipe from: "Maximum size that can be buffered ever." To: "Maximum size that can be buffered ahead of all reader input streams." The distinction means that we can increase the buffer to accommodate cloned input streams that are not being read at the same rate. We effectively buffer data up to: "Specified maximum + (read position of fasted input stream - read position of slowest input stream)" In effect, we apply back pressure to the writer based only on the read position of the fastest input stream. Ideally code would consume both streams at similar rates, but we cannot always guarantee that. In particular, the pipe clone backs our Response.clone() method which is exposed to script. We cannot control if script clones a stream and then does not read one side of it. The only other real option for solving this are to create an infinite pipe and copy to it when a clone occurs. That is a worse solution, however, because it allows unlimited data to be buffered in advance of all reader input streams. It basically removes back pressure completely. I tried to be liberal with my comments in the code since this stuff is obviously complex. I also have gtests that exercise the new behavior in P2.

Comment on attachment 8815794 [details] [diff] [review] P2 Verify that cloned pipe streams can be read at different rates. r=froydnj This modifies the gtests to exercise the behavior described in comment 19.

Comment on attachment 8815794 [details] [diff] [review] P2 Verify that cloned pipe streams can be read at different rates. r=froydnj Review of attachment 8815794 [details] [diff] [review]: ----------------------------------------------------------------- ::: xpcom/tests/gtest/TestPipes.cpp @@ +856,5 @@ > reader->Close(); > > + // Because the clone stream is still buffered the writable callback should > + // not be fired. > + //ASSERT_FALSE(cb->Called()); Whoops. This should not be commented out. New patch coming.

I fixed the commented out line. I also added a test for a corner case I thought of. Consider: 1. You have an input stream that has been completely read. 2. You have a cloned stream that has not been read and is buffering data in excess of the configured "max". 3. The pipe is considered writable at this point. 4. The drained input stream is closed. 5. The pipe should become blocked and unwritable since there is now only one stream that is buffered beyond the limit. This condition was not possible before the P1 or cloned streams in general. Previously closing the reader input stream would always close the writer output stream as well. Anyway, I don't think this quirk is easily observed if you only have access to the output stream handle. As far as that code knows the pipe filled up. It doesn't know whether its because it hit the limit or because one side of clone was closed.

Because this effects a compat issue we are getting reports on I'd like to uplift it where possible. At the very least to FF52 aurora so its in the next ESR. Also potentially FF51 beta. Normally I would not uplift a pipe change like this to beta, but since we will have an extra 6 weeks in the beta cycle for FF51 I think its reasonable. I intend to let it bake on nightly at least a week before any uplift, though.

Comment on attachment 8815795 [details] [diff] [review] P1 Allow pipe cloned streams to be read at different rates. r=froydnj Review of attachment 8815795 [details] [diff] [review]: ----------------------------------------------------------------- The comments were very helpful, thank you for including them. ::: xpcom/io/nsPipe3.cpp @@ +310,5 @@ > > // > + // Methods below may only be called while inside the pipe's monitor. Some > + // of these methods require passing a ReentrantMonitorAutoEnter to prove > + // monitor is held. Nit: "to prove the monitor is held." @@ +325,5 @@ > void RollBackAllReadCursors(char* aWriteCursor); > void UpdateAllReadCursors(char* aWriteCursor); > void ValidateAllReadCursors(); > + uint32_t GetBufferSegmentCount(const ReentrantMonitorAutoEnter& ev, > + const nsPipeReadState& aReadState) const; Was there a reason you put the monitor argument first here? I've usually seen it done as the last argument, but I suppose it doesn't matter much. @@ +502,5 @@ > // > +// Likewise, each input stream reader will have its own amount of > +// buffered data. The pipe size threshold, however, is only applied > +// to the input stream that is being read fastest. We call this > +// the "advance buffer" in that its in advance of all readers. We Nit: "it's" This comment talks about "the input stream being read fastest" and calls that "the advance buffer", implying singleness, but the pipe has an mMaxAdvanceBufferSegmentCount variable, implying a multiplicity of advance buffers. Maybe it's just me, but that seems confusing. Can you rewrite this comment to make the terminology more clear? @@ +1143,5 @@ > + // The write segment can be smaller than the current reader position > + // in some cases. For example, when the first write segment has not > + // been allocated yet mWriteSegment is negative. In these cases > + // the stream is effectively using zero segments. > + if (mWriteSegment < aReadState.mSegment) { Should we MOZ_ASSERT(mWriteSegment >= 0) (and maybe MOZ_ASSERT(aReadState.mSegment >= 0) after this conditional block, then? Perhaps we should turn mWriteSegment into a Maybe<uint32_t> so we know for sure when it's valid and when it's not?

Comment on attachment 8815839 [details] [diff] [review] P2 Verify that cloned pipe streams can be read at different rates. r=froydnj Review of attachment 8815839 [details] [diff] [review]: ----------------------------------------------------------------- Again, excellent comments.

Thanks for the review! (In reply to Nathan Froyd [:froydnj] from comment #24) > > + uint32_t GetBufferSegmentCount(const ReentrantMonitorAutoEnter& ev, > > + const nsPipeReadState& aReadState) const; > > Was there a reason you put the monitor argument first here? I've usually > seen it done as the last argument, but I suppose it doesn't matter much. No reason particular reason. If there is a convention to do it the other way I can switch it. > > +// Likewise, each input stream reader will have its own amount of > > +// buffered data. The pipe size threshold, however, is only applied > > +// to the input stream that is being read fastest. We call this > > +// the "advance buffer" in that its in advance of all readers. We > This comment talks about "the input stream being read fastest" and calls > that "the advance buffer", implying singleness, but the pipe has an > mMaxAdvanceBufferSegmentCount variable, implying a multiplicity of advance > buffers. Maybe it's just me, but that seems confusing. Can you rewrite > this comment to make the terminology more clear? So the "Count" here is the number of segments in the single advance buffer. I'll try to clarify that with more comments. I could also change it to mMaxAdvanceBufferSegments, but I find "Count" easier to see/type than a single plural "s". > @@ +1143,5 @@ > > + // The write segment can be smaller than the current reader position > > + // in some cases. For example, when the first write segment has not > > + // been allocated yet mWriteSegment is negative. In these cases > > + // the stream is effectively using zero segments. > > + if (mWriteSegment < aReadState.mSegment) { > > Should we MOZ_ASSERT(mWriteSegment >= 0) (and maybe > MOZ_ASSERT(aReadState.mSegment >= 0) after this conditional block, then? I'll try that. > Perhaps we should turn mWriteSegment into a Maybe<uint32_t> so we know for > sure when it's valid and when it's not? I think that would be a non-trivial change. It might be worth doing, but I'd prefer to have it explored in a separate bug.

Pushed by bkelly@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/f5ea92f348b1 P1 Allow pipe cloned streams to be read at different rates. r=froydnj https://hg.mozilla.org/integration/mozilla-inbound/rev/d366e6c749d6 P2 Verify that cloned pipe streams can be read at different rates. r=froydnj

https://hg.mozilla.org/mozilla-central/rev/f5ea92f348b1 https://hg.mozilla.org/mozilla-central/rev/d366e6c749d6

(In reply to Ben Kelly [:bkelly] from comment #26) > > Perhaps we should turn mWriteSegment into a Maybe<uint32_t> so we know for > > sure when it's valid and when it's not? > > I think that would be a non-trivial change. It might be worth doing, but > I'd prefer to have it explored in a separate bug. Of course. Would you please file that?

(In reply to Nathan Froyd [:froydnj] from comment #29) > Of course. Would you please file that? Sure. Filed as bug 1322272.

Comment on attachment 8815795 [details] [diff] [review] P1 Allow pipe cloned streams to be read at different rates. r=froydnj Approval Request Comment [Feature/Bug causing the regression]: Web Compat issue in service worker Cache and Fetch APIs. [User impact if declined]: Its a web compat problem that could cause sites to fail in firefox while they work in chrome. [Is this code covered by automated tests?]: nsPipe is used throughout the codebase, so its heavily tested by our overall automated test suite. I also added new tests in P2 for the specific issue fixed here. [Has the fix been verified in Nightly?]: The fix has been in nightly for about a week and a half. No issues have been reported. [Needs manual test from QE? If yes, steps to reproduce]: No. [List of other uplifts needed for the feature/fix]: None. [Is the change risky?]: Its moderately risky given how important nsPipe is to the overall codebase. I am requesting uplift at the beginning of the cycle so we have a full 6 weeks to catch any problems. [Why is the change risky/not risky?]: A lot of our code uses nsPipe. Due to multi-threading its inherently subject to timing issues and races. None of the existing tests, new tests, or time on nightly have shown any such problems, though. I put diagnostic assertions in the code, so if there were problems I would expect a spike in nightly crash reports related to this code. All of this leads me to believe the risk is worth fixing the web compat problem. [String changes made/needed]: None.

Comment on attachment 8815839 [details] [diff] [review] P2 Verify that cloned pipe streams can be read at different rates. r=froydnj See comment 31.

Comment on attachment 8815795 [details] [diff] [review] P1 Allow pipe cloned streams to be read at different rates. r=froydnj fix webcompat issue with service workers in aurora52

Comment on attachment 8815839 [details] [diff] [review] P2 Verify that cloned pipe streams can be read at different rates. r=froydnj add tests, aurora52+

Comment on attachment 8815795 [details] [diff] [review] P1 Allow pipe cloned streams to be read at different rates. r=froydnj Fix a webcompat issue and add test. Beta51+. Should be in 51 beta 8.

https://hg.mozilla.org/releases/mozilla-aurora/rev/dec6908254a5 https://hg.mozilla.org/releases/mozilla-aurora/rev/b512f512ba1f

https://hg.mozilla.org/releases/mozilla-beta/rev/73a35ef26a49 https://hg.mozilla.org/releases/mozilla-beta/rev/5045ce8f3c06