Improve set_position to reuse buffer.

[lateapexearlyspeed]

Fix #54593.
Main purpose is to try to keep buffer valid as possible so that buffer data can still be used after set_position within range [beginBufferPosMapToUnderlyingStream, underlyingStream.Position)

[ghost]

Tagging subscribers to this area: @dotnet/area-system-io
See info in area-owners.md if you want to be subscribed.

Issue Details

---

Fix #54593.
Main purpose is to try to keep buffer valid as possible so that buffer data can still be used after set_position within range [beginBufferPosMapToUnderlyingStream, underlyingStream.Position)

Author:	lateapexearlyspeed
Assignees:	-
Labels:	area-System.IO
Milestone:	-

[lateapexearlyspeed]

This PR is ready for review now, please review, thanks.

Description:

1. Improve set_posittion: Make buffer data valid if set_posittion inside range of current buffer
2. Refactor existing implementation of Seek() so that Seek() and set_position can use new common logic (I feel new logic can cover existing one and simplify code. If not suitable, we can revert this part2)
3. Add test case which covers both set_position and Seek to verify Reading after changing position will not trigger underlying stream's Read while keep other operations correct.

[jeffhandley]

@carlossanlop This PR is assigned to you for follow-up/decision before the RC1 snap.

[jeffhandley]

I'm going to merge from main, resolve the conflicts, and push to this PR's branch...

[lateapexearlyspeed]

Okay and thanks for resolving conflict, this PR was pending long time :)

[jeffhandley]

LGTM; I'll push in the (preexisting) typo fix. I'd like another reviewer to take a look before we merge it though.

[jeffhandley]

The test failure is unrelated (as noted by @karelz). We'll just wait for an approval from @adamsitnik, @carlossanlop, or @stephentoub whenever one of them is available. The target is merging this by August 15th. Thanks for your patience, @lateapexearlyspeed.

[adamsitnik]

I was not aware of the fact that Seek was capable of reusing the buffer while Position setter was not. Since we are very close to release of .NET 6 and I don't feel comfortable introducing any non trivial changes to the buffering logic (I am speaking from experience, as I've blocked preview 4 release with a bug in buffering logic myself: #51151) I would prefer to reuse the existing Seek logic by simply calling Seek from Position setter.

@lateapexearlyspeed please don't get me wrong. I am not saying that your refactor and logic changes have a bug, I just don't feel 100% comfortable merging it now.

[stephentoub]

I would prefer to reuse the existing Seek logic by simply calling Seek from Position setter

set_Position already calls Seek; it just first zeros out some state... it's not clear to me why it does that, but it would seem the right answer here would just be to delete code from the setter:

            set
            {
                if (value < 0)
                    ThrowHelper.ThrowArgumentOutOfRangeException(ExceptionArgument.value, ExceptionResource.ArgumentOutOfRange_NeedNonNegNum);
 
-                EnsureNotClosed();
-                EnsureCanSeek();
- 
-                if (_writePos > 0)
-                    FlushWrite();
- 
-                _readPos = 0;
-                _readLen = 0;
                _stream!.Seek(value, SeekOrigin.Begin);
            }

Is that sufficient to address the issue?

[adamsitnik]

set_Position already calls Seek

it calls _stream!.Seek, while it should call this.Seek(value, SeekOrigin.Begin) which contains the logic that we care about

runtime/src/libraries/System.Private.CoreLib/src/System/IO/BufferedStream.cs

Lines 1229 to 1244 in 4566436

	// If the seek destination is still within the data currently in the buffer, we want to keep the buffer data and continue using it.
	// Otherwise we will throw away the buffer. This can only happen on read, as we flushed write data above.
	// The offset of the new/updated seek pointer within _buffer:
	_readPos = (int)(newPos - (oldPos - _readPos));
	// If the offset of the updated seek pointer in the buffer is still legal, then we can keep using the buffer:
	if (0 <= _readPos && _readPos < _readLen)
	{
	// Adjust the seek pointer of the underlying stream to reflect the amount of useful bytes in the read buffer:
	_stream.Seek(_readLen - _readPos, SeekOrigin.Current);
	}
	else
	{ // The offset of the updated seek pointer is not a legal offset. Loose the buffer.
	_readPos = _readLen = 0;
	}

[stephentoub]

it calls _stream!.Seek, while it should call this.Seek(value, SeekOrigin.Begin) which contains the logic that we care about

Ah, I missed the _stream. part. Then yeah, that last line should change as well.

            set
            {
                if (value < 0)
                    ThrowHelper.ThrowArgumentOutOfRangeException(ExceptionArgument.value, ExceptionResource.ArgumentOutOfRange_NeedNonNegNum);
 
-                EnsureNotClosed();
-                EnsureCanSeek();
- 
-                if (_writePos > 0)
-                    FlushWrite();
- 
-                _readPos = 0;
-                _readLen = 0;
-                _stream!.Seek(value, SeekOrigin.Begin);
+                Seek(value, SeekOrigin.Begin);
            }

[adamsitnik]

I've applied the suggestion I had in order to merge it before we snap for RC1. Once again thank you @lateapexearlyspeed !

[ghost]

Hello @adamsitnik!

Because this pull request has the auto-merge label, I will be glad to assist with helping to merge this pull request once all check-in policies pass.

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (@msftbot) and give me an instruction to get started! Learn more here.