Provide documentation and context information for NIOTooManyBytesError #2831

cmcgee1024 · 2024-08-07T17:29:46Z

Motivation:

The NIOTooManyBytesError doesn't have any documentation for someone that encounters this error. Also, they have no idea the magnitude of the limit that was set to decide if the payload is an unreasonable size.

Modifications:

Provide documentation that explains the situation when the error occurs, which is the upTo limit of an AsyncSequence is exceeded. Describe one potential action, which is to increase this limit.

Provide the maxBytes in the error so that the user can gauge whether the upTo limit is already reasonable and the payload size is excessive, or the limit needs to be increased to suit more situations.

Result:

There will be documentation for the NIOTooManyBytesError so that if someone looks it up they will have a better understanding of the situation, and possible actions. The limit will be included in the error to give them an idea of the scale of the payload limit that is in place, or potentially the configuration value that they can change.

The NIOTooManyBytesError doesn't have any documentation for someone that encounters this error. Provide documentation that explains the situation when the error occurs, which is the upTo limit of an AsyncSequence is exceeded. Describe one potential action, which is to increase this limit. The error doesn't give the end user any sense of magnitude of the byte limit that has been exceeded. Provide the maxBytes in the error so that the user can gauge whether the upTo limit is already reasonable and the payload size is excessive, or the limit needs to be increased to suit more situations.

cmcgee1024 · 2024-08-21T20:00:51Z

@weissi would it preferable to provide a backward compatible init function and make maxBytes optional?

weissi · 2024-08-21T20:02:32Z

@weissi would it preferable to provide a backward compatible init function and make maxBytes optional?

Yes, we should not break SemVer major just for this. I'll let @Lukasa & @FranzBusch et al comment more on what API would be good

dnadoba

We likely want to make two of these errors equal regardless of their value of the maxBytes property.

dnadoba · 2024-08-21T20:47:59Z

Sources/NIOCore/AsyncAwaitSupport.swift

 public struct NIOTooManyBytesError: Error, Hashable {
-    public init() {}
+    let maxBytes: Int?


This should be a var. no need to restrict it to a let as there are no invariants or similar that need to be verified.

Also this should be public.

If it's var and public, then what's a good way to prevent it from being mutated after the initial throw?

There's no reason to prevent it being mutated. And indeed, given the init, you can't:

extension NIOTooManyBytesError { mutating func getOutOfMyWayFoolsIMutateWhatIWant(maxBytes: Int?) { self = .init(maxBytes: maxBytes) } }

Ok, I've updated it to be public mutable.

dnadoba · 2024-08-21T20:49:37Z

Sources/NIOCore/AsyncAwaitSupport.swift

+/// more bytes in a sequence than what was specified as the maximum. It could be that this upTo
+/// limit should be increased, or that the sequence has unexpected content in it.
+///
+/// - maxBytes: (optional) current limit on the maximum number of bytes in the sequence


The docs for a property should be above the property.

Lukasa

This generally looks good, but I'd like to see the tests updated to validate that this is working properly.

Sources/NIOCore/AsyncAwaitSupport.swift

Lukasa · 2024-08-22T10:14:01Z

Sources/NIOCore/AsyncAwaitSupport.swift

 public struct NIOTooManyBytesError: Error, Hashable {
-    public init() {}
+    let maxBytes: Int?


Also this should be public.

… and test maxBytes is set as expected

Lukasa · 2024-08-23T11:49:00Z

Tests/NIOCoreTests/AsyncSequenceTests.swift

@@ -138,14 +139,22 @@ final class AsyncSequenceCollectTests: XCTestCase {
                    file: testCase.file,
                    line: testCase.line
                )
+                if let tooManyBytesErr = error as? NIOTooManyBytesError {


This should fail the test if the cast doesn't pass.

Lukasa · 2024-08-23T11:49:11Z

Tests/NIOCoreTests/AsyncSequenceTests.swift

@@ -154,6 +163,14 @@ final class AsyncSequenceCollectTests: XCTestCase {
                    file: testCase.file,
                    line: testCase.line
                )
+                if let tooManyBytesErr = error as? NIOTooManyBytesError {


This should fail the test if the cast doesn't pass.

dnadoba · 2024-08-23T13:00:33Z

Sources/NIOCore/AsyncAwaitSupport.swift

+/// exceeds a certain threshold beyond which the content isn't matching an expected reasonable
+/// size to be processed. This error is generally thrown when it is discovered that there are more
+/// more bytes in a sequence than what was specified as the maximum. It could be that this upTo
+/// limit should be increased, or that the sequence has unexpected content in it.
 public struct NIOTooManyBytesError: Error, Hashable {


Can you add a custom Equatable and Hashable implementation that ignores maxBytes? == can just return true and hash(into:) can just hash a constant e.g. 0. Users are usually not interested in the specific maxBytes value but rather just that it is a NIOTooManyBytesError error.

dnadoba

Thanks! LGTM

cmcgee1024 · 2024-08-27T13:41:16Z

After looking through the various failures I see that there's a formatter failure. How best to run the formatter for this project to reformat the code? Homebrew installing the swift-format package yields an error about an unrecognized rule.

weissi added the ⚠️ semver/major Breaks existing public API. label Aug 9, 2024

weissi requested a review from dnadoba August 21, 2024 20:02

Maintain API compatibility

48c4858

dnadoba reviewed Aug 21, 2024

View reviewed changes

Lukasa requested changes Aug 22, 2024

View reviewed changes

cmcgee1024 added 3 commits August 22, 2024 11:44

Deprecate the original constructor, make the maxBytes public mutable,…

3c372bf

… and test maxBytes is set as expected

Remove foundation import

3623af2

Tighten up the description of the error

60f2764

Lukasa reviewed Aug 23, 2024

View reviewed changes

dnadoba reviewed Aug 23, 2024

View reviewed changes

Code review feedback

f065562

dnadoba approved these changes Aug 23, 2024

View reviewed changes

Lukasa added 🆕 semver/minor Adds new public API. and removed ⚠️ semver/major Breaks existing public API. labels Aug 27, 2024

Lukasa approved these changes Aug 27, 2024

View reviewed changes

Merge branch 'main' into too-many-bytes

7d885a4

cmcgee1024 and others added 2 commits August 27, 2024 09:55

Fix formatting errors

6a4b8af

Merge branch 'main' into too-many-bytes

e261aa1

Lukasa merged commit e1b2a99 into apple:main Aug 28, 2024
27 of 29 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Provide documentation and context information for NIOTooManyBytesError #2831

Provide documentation and context information for NIOTooManyBytesError #2831

cmcgee1024 commented Aug 7, 2024

cmcgee1024 commented Aug 21, 2024

weissi commented Aug 21, 2024

dnadoba left a comment

dnadoba Aug 21, 2024

Lukasa Aug 22, 2024

cmcgee1024 Aug 22, 2024

Lukasa Aug 22, 2024

cmcgee1024 Aug 22, 2024

dnadoba Aug 21, 2024

Lukasa left a comment

Lukasa Aug 22, 2024

Lukasa Aug 23, 2024

Lukasa Aug 23, 2024

dnadoba Aug 23, 2024

dnadoba left a comment

cmcgee1024 commented Aug 27, 2024 •

edited

Loading

Provide documentation and context information for NIOTooManyBytesError #2831

Provide documentation and context information for NIOTooManyBytesError #2831

Conversation

cmcgee1024 commented Aug 7, 2024

Motivation:

Modifications:

Result:

cmcgee1024 commented Aug 21, 2024

weissi commented Aug 21, 2024

dnadoba left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Lukasa left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dnadoba left a comment

Choose a reason for hiding this comment

cmcgee1024 commented Aug 27, 2024 • edited Loading

cmcgee1024 commented Aug 27, 2024 •

edited

Loading