Adopt NIOThrowingAsyncSequenceProducer 2nd try #2917
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
glbrntt
requested changes
Oct 14, 2024
Comment on lines
118
to
119
| lowWatermark: 4, | ||
| highWatermark: 8 |
There was a problem hiding this comment.
We pass in the watermarks to makeFileChunksStream so presumably we should use them here?
Comment on lines
454
to
457
| internal enum RangeUpdateOutcome { | ||
| case moreToRead | ||
| case cannotReadMore | ||
| } |
There was a problem hiding this comment.
nit: can you move this to the extension where its used?
| return .success((descriptor, state.range)) | ||
| } else { | ||
| case .producing(let producingState), .pausedProducing(let producingState): | ||
| guard let descriptor = producingState.handle.descriptorIfAvailable() else { |
There was a problem hiding this comment.
nit: I don't think a guard is any clearer here as the exit on the failure path isn't earlier than on the success path. IMO if both paths immediately return then if/else is clearer.
| /// Updates the range (the offsets to read from and up to) to reflect the number of bytes which have been read. | ||
| /// - Parameter count: The number of bytes which have been read. | ||
| /// - Returns: Returns `True` if there are no remaining bytes to read, `False` otherwise. | ||
| mutating func updateRangeWithReadBytes(_ count: Int) -> ProducerState.RangeUpdateOutcome { |
There was a problem hiding this comment.
This is definitely clearer than the existing in-line range updates 🙂
| // update range, we are not done | ||
| self.range = newLowerBound..<currentRange.upperBound | ||
| if currentRange.upperBound - newLowerBound < 1 { | ||
| fatalError("here \(newLowerBound) \(currentRange.upperBound)") |
Motivation: Adopt `NIOThrowingAsyncSequenceProducer` in NIOFileSystem to reduce code duplication. Modifications: Adopt `NIOThrowingAsyncSequenceProducer` in NIOFileSystem `DirectoryEntryProducer` and `FileChunkProducer` Result: No functional changes. Internal changes reduce code duplication.
In the original adoption of `NIOThrowingAsyncSequenceProducer` the code did not deal with backpressure being applied very well. A key bug was that it was not protected against re-entrant calls to `produceMore`. Such calls may happen e.g. when the producer has been asked to pause producing and then resume again before the initial read had completed. This resulted in overlapping reads and re-ordered data. This change introduces a new activity state which is protected by a lock and keeps track of if we are in the critical section to serialize access. `DirectoryEntryProducer` performs most of its logic within a lock and so doesn't seem to be impacted in the same way.
rnro
force-pushed
the
adopt_niothrowingasyncsequenceproducer_2nd_try
branch
from
b83970c
to
8e8bf85
Compare
glbrntt
approved these changes
Oct 15, 2024
rnro
force-pushed
the
adopt_niothrowingasyncsequenceproducer_2nd_try
branch
from
2ae78e6
to
51720a8
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Motivation:
Adopt
NIOThrowingAsyncSequenceProducerin NIOFileSystem to reduce codeduplication.
Modifications:
Adopt
NIOThrowingAsyncSequenceProducerin NIOFileSystemDirectoryEntryProducerandFileChunkProducer.This change was previously merged and then backed out due to issues (#2879). The original change is in the first commit, the second commit contains additional changes.
In the original adoption of
NIOThrowingAsyncSequenceProducerthe code did not deal with backpressure being applied very well, in some cases causes hangs.A key bug was that it was not protected against re-entrant calls to
produceMore. Such calls may happen e.g. when the producer has been asked to pause producing and then resume again before the initial read had completed. This resulted in overlapping reads and re-ordered data.This change introduces a new activity state which is protected by a lock and keeps track of if we are in the critical section to serialize access.
DirectoryEntryProducerperforms most of its logic within a lock and sodoesn't seem to be impacted in the same way.
Result:
No functional changes. Internal changes reduce code duplication.