PagedCharBuffer, Append(string value), never ending loop

[montigab78]

In file PagedCharBuffer.cs, method public void Append(string value), the loopwhile (count > 0) at line 56 doed not exit in some scenarios. As a result, the web application hangs.

The problem is that GetCurrentPage and NewPage calls within the loop make use of ArrayPool.RentUse (public abstract T[] Rent(int minimumLength) which takes one parameter, which is the minimumLength. Sometimes the actual length of the returned array is greater than requested one (1024 corresponding to PageSize const). As soon as this happens (apparently when Append argument is a quite long string) any further invokation of GetCurrentPage fails to get new pages.
In fact, if (CurrentPage == null || _charIndex == PageSize returns false as _charIndex is now set to a different value than the expected PageSize (2048 whenever I experienced the bug).
I solved the bug by changing line 59 like this var copyLength = Math.Min(count, PageSize - _charIndex);

[davidfowl]

Do you have a sample page/app that is broken?

[montigab78]

I'm not able to provide the application as it involves a database and other stuff (images, etc.). I believe the problem arises when the web page generated code grows and contains >50k of JSON (serialized model). I recall that when the never ending loop started we had about 50 arrays (pages) of 1k each.

However, the code In that loop is error prone as the size of the resulting page is not fixed.

[Eilon]

@dougbu can you take a look?

[dougbu]

@Eilon what's probably happening is a PagedCharBuffer is exhausting the underlying ArrayPool<char>+Bucket for arrays of length 1024 and falling back to the Bucket for arrays of length 2048. Inconsistent bounds checking in PagedCharBuffer shows its face soon after.

@psicus78's suggestion should work fine as long as we apply it to the Append(char[], int, int) method as well. A more memory-efficient choice would be to use page.Length in all conditions i.e. to utilize the whole of the char[2048] or char[4096] arrays that may be returned after fallbacks. But that will complicate the Length calculation.

Repro scenario involves writing at least 50k characters to a PagedCharBuffer backed by ArrayPool<char>.Shared without clearing the buffer. That could occur when writing a large view or set of views to the response Stream.

[dougbu]

Clearing investigate label because I've written tests showing my description above is correct. Have fix if we want it for 1.1.0.

[montigab78]

Great! I hope this will be shipped by 1.1.0 because it's a big problem for my application.

[rynowak]

@psicus78 is this broken for you in 1.0.0-1.0.1? Or are you using 1.1.0?

@Eilon If this is broken in LTS this seems like a good patch candidate.

[montigab78]

@rynowak I'm using dotnet.myget.org/aspnetcore 1.1.0-alpha-* packages because I need signalR which is still alpha.

[dougbu]

@rynowak PagedCharBuffer wasn't in 1.0.1. @pranavkm added it in early September.

The patch question should be about similar code we have elsewhere. Fortunately we don't use ArrayPool<T> too many places.

[dougbu]

similar code we have elsewhere

No obvious bounds checking issues related to ArrayPool<T> use in rel/1.0.1.

It's good I looked because our current code also uses incorrect bounds in CacheTagHelper+CharBufferHtmlContent.WriteTo() and PagedBufferedTextWriter.FlushAsync(). They both could leave out big pieces of early segments and attempt to grab too much from the last one.

[dougbu]

Fixed missing word in previous comment.

[Eilon]

Spoke to folks in person. It appears that this cannot repro on 1.0.x, so we'll just fix in 1.1.0.

[dougbu]

4e6fd82