Decouple PgReader GetTextReader and GetChars #6490
Conversation
There was a problem hiding this comment.
Pull request overview
This PR relaxes some restrictions around concurrently active read operations in non-sequential mode by decoupling PgReader’s GetTextReader implementation from column-stream tracking (and by treating GetChars’ internal stream usage as an implementation detail). This aligns behavior more closely with GetBytes semantics and supports future cleanup work around PgReader.GetStream.
Changes:
- Adjusts the
GetTextReadertest to only expect “second text reader on same column throws” under sequential access. - Refactors
PreparedTextReaderto no longer depend on / dispose an underlyingColumnStream. - Updates
PgReaderto separate “user-facing stream” tracking fromGetCharsinternal reading, and ensures cleanup also disposes_preparedTextReader.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| test/Npgsql.Tests/ReaderTests.cs | Makes the “GetTextReader twice” assertion conditional on sequential mode. |
| src/Npgsql/PreparedTextReader.cs | Removes ColumnStream coupling and stream-disposal behavior from PreparedTextReader. |
| src/Npgsql/Internal/PgReader.cs | Adjusts stream creation/tracking and GetTextReader/GetChars plumbing; improves cleanup logic. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
NinoFloris
force-pushed
the
decouple-textreader
branch
from
29451ea to
d668b13
Compare
# Conflicts: # src/Npgsql/NpgsqlDataReader.cs
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
Comments suppressed due to low confidence (1)
src/Npgsql/Internal/PgReader.cs:674
- Restart() no longer calls Cleanup() when switching from resumable to non-resumable and rewinding the field. If a user-facing stream/text reader is still open, Restart will rewind the underlying buffer position without invalidating that stream, leaving it in an inconsistent state. Suggest explicitly handling active user streams here (dispose/throw) or otherwise guaranteeing that any user-facing readers are independent of PgReader buffer position before allowing rewinds.
// From this point on we're not resuming, we're resetting any previous converter state and rewinding our position.
if (NestedInitialized)
ResetCurrent();
_fieldConsumed = false;
_resumable = resumable;
RewindCore(FieldOffset);
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // This will cause any previously handed out StreamReaders etc to throw, as intended. | ||
| if (_userActiveStream is not null) | ||
| if (!forGetChars && StreamActive) | ||
| DisposeUserActiveStream(async: false).GetAwaiter().GetResult(); | ||
|
|
||
| length ??= CurrentRemaining; | ||
| CheckBounds(length.GetValueOrDefault()); | ||
| return _userActiveStream = _buffer.CreateStream(length.GetValueOrDefault(), canSeek && length <= _buffer.ReadBytesLeft, consumeOnDispose: false); | ||
| var stream = _buffer.CreateStream(length.GetValueOrDefault(), canSeek && length <= _buffer.ReadBytesLeft, consumeOnDispose: false); | ||
|
|
||
| // GetChars isn't a user facing stream and handles its own tracking and cleanup. | ||
| if (!forGetChars) | ||
| { | ||
| _requiresCleanup = true; | ||
| _userActiveStream = stream; | ||
| } | ||
| return stream; |
There was a problem hiding this comment.
GetColumnStream(forGetChars: true) can now create a second ColumnStream while a user-facing stream/text reader is still active. Since ColumnStream reads by advancing the shared NpgsqlReadBuffer.ReadPosition, having two active streams (or rewinding the buffer while one is active) can corrupt the other reader/stream (e.g., a large GetTextReader that returns StreamReader, then GetChars triggers a restart/rewind). Consider either rejecting this scenario (throw) or ensuring GetChars uses an independent buffered source (no shared ReadPosition) when a user stream/text reader is active.
There was a problem hiding this comment.
We should switch to an isolated implementation for non-sequential streams, as we know there won't be any I/O.
| if (IsSequential) | ||
| { | ||
| Assert.That(async () => await textReaderGetter(reader, 0), | ||
| Throws.Exception.TypeOf<InvalidOperationException>(), | ||
| "Sequential text reader twice on same column"); | ||
| } | ||
| else | ||
| { | ||
| Assert.That(reader.GetChars(0, 0, actual, 4, 1), Is.EqualTo(1)); | ||
| } |
There was a problem hiding this comment.
The non-sequential branch now exercises GetChars while a TextReader is open, but only with a very small value that will go through the PreparedTextReader path. The behavior being changed is most fragile when GetTextReader returns a StreamReader (value > PgReader.MaxPreparedTextReaderSize), so adding a variant of this test with a >64KB string would better cover the new concurrency behavior.
# Conflicts: # src/Npgsql/Internal/PgReader.cs
We haven't left the column so we can keep open any resources pointing into it # Conflicts: # src/Npgsql/Internal/PgReader.cs
NinoFloris
force-pushed
the
decouple-textreader
branch
from
d668b13 to
a4f9813
Compare
|
Taking this to draft until I have some time to rework our non-sequential column streams. |
Based on #5480
Loosening restrictions on concurrently active operations in non-sequential mode a bit. Needed to make one assert conditional on sequential mode for tests to pass, though the assert message already implied it should have been conditional from the start.
This PR basically changes the fact that GetChars needs a column stream to an internal implementation detail (it's still being cleaned up on column commit etc).
It will help with future cleanup work around PgReader.GetStream, and it makes behavior a bit more regular in non-sequential mode, as GetBytes doesn't dispose column streams returned to users either.