Make ObjectiveCError type Sendable without warnings by dfed · Pull Request #73 · dfed/CacheAdvance

dfed · 2022-11-05T00:25:25Z

Error inherits from Sendable, but NSException does not conform to Sendable (probably due to the raise() method?). In order to remove a warning that ObjectiveCError does not actually conform to Sendable on Xcode 14+, I made ObjectiveCError include the exception name and the reason.

Since ObjectiveCError is not public, this is not a breaking change.

codecov · 2022-11-05T00:40:35Z

Codecov Report

Merging #73 (d051e71) into main (61f099f) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##             main      #73   +/-   ##
=======================================
  Coverage   98.92%   98.92%           
=======================================
  Files          13       13           
  Lines         556      560    +4     
=======================================
+ Hits          550      554    +4     
  Misses          6        6

Impacted Files	Coverage Δ
Sources/CacheAdvance/CacheReader.swift	`93.67% <100.00%> (+0.16%)`	⬆️
Sources/CacheAdvance/ObjectiveC.swift	`100.00% <100.00%> (ø)`

dfed · 2022-11-10T02:44:14Z

            }
        }
-        if let endOffset = endOffset, offsetInFile != endOffset {
+        if let endOffset = endOffset, offsetInFile < endOffset {


This change is unrelated to this PR. It addresses this comment on #72.

To make sure I follow... we'd end up throwing an error from this section of the code if the end offset was past EOF?

Do you think we're losing any safety here by changing to <? I was imagining we'd add a comment to state our intention but leave the check the way it was.

I can't imagine how we could end up with the > here but if we did it seems like that would mean we also have a corrupted file.

We check for the > case a few lines above:

CacheAdvance/Sources/CacheAdvance/CacheReader.swift

Lines 56 to 58 in 61f099f

} else if offsetInFile > endOffset {

throw CacheAdvanceError.fileCorrupted

}

We aren't losing safety today by making this change, and with our current unit tests I don't believe we are losing future safety. Though if someone were to change the unit tests and modify this code we could end up with a future issue, but that doesn't seem terribly likely.

That said, I should absolutely add a comment.

@dfed the concern I had was that if there was a bug in nextEncodedMessage() where we could end up in a situation (in theory) where we got to L61 and offsetInFile > endOffset. This could occur if the file were corrupted AND nextEncodedMessage() returned nil even though it advanced the reader.

Where I personally netted out was that checking offsetInFile != endOffset wasn't wrong but resulted in it being difficult for the reader to follow why the check existed. And I felt like that difficulty was resolved by the comment.

I'm open to the idea that this line of thinking is being too cautious though. I wanted to take a moment to voice my thought though I'm also supportive of merging this PR as-is.

Since @jianjunwoo has been doing great thinking here and authored this code I'll also tag him here for visibility 👍

the concern I had was that if there was a bug in nextEncodedMessage() where we could end up in a situation (in theory) where we got to L61 and offsetInFile > endOffset. This could occur if the file were corrupted AND nextEncodedMessage() returned nil even though it advanced the reader.

This should not be possible, since the only way we return nil from nextEncodedMessage() is if we get an empty read (which would not advance the reader). That said, that's true today and may not be true forever.

I'll update this code + comment to be a bit more clear, then merge.

"That said, that's true today and may not be true forever."

This is true : D

Overall, it looks fine in either way for me. There are unit tests to cover these cases I think. We've got noticed when it is not true.

On the other side, when the end offset is larger than the EOF, it could be a warning instead of an error to identify the incorrect endOffset. In this case, the messages are still valid. Currently, we treat this case in a strict way.

On the other side, when the end offset is larger than the EOF, it could be a warning instead of an error to identify the incorrect endOffset. In this case, the messages are still valid. Currently, we treat this case in a strict way.

You're right! Prior to #66 we wouldn't have caught this case either. Though because of the == check we have, the only way to hit the offsetInFile > endOffset case is for something to have gone horribly wrong (like, more than just an untimely crash), so I think it's valid that we consider files in this state corrupted.

dfed · 2022-11-10T04:43:46Z

@bachand @fdiaz once this PR is approved and merged I'll release the next version bump on Cocoapods.

bachand · 2022-11-12T02:44:54Z

            }
        }
-        if let endOffset = endOffset, offsetInFile != endOffset {
+        if let endOffset = endOffset, offsetInFile < endOffset {


To make sure I follow... we'd end up throwing an error from this section of the code if the end offset was past EOF?

bachand · 2022-11-18T02:14:00Z

            }
        }
-        if let endOffset = endOffset, offsetInFile != endOffset {
+        if let endOffset = endOffset, offsetInFile < endOffset {


@dfed the concern I had was that if there was a bug in nextEncodedMessage() where we could end up in a situation (in theory) where we got to L61 and offsetInFile > endOffset. This could occur if the file were corrupted AND nextEncodedMessage() returned nil even though it advanced the reader.

Where I personally netted out was that checking offsetInFile != endOffset wasn't wrong but resulted in it being difficult for the reader to follow why the check existed. And I felt like that difficulty was resolved by the comment.

I'm open to the idea that this line of thinking is being too cautious though. I wanted to take a moment to voice my thought though I'm also supportive of merging this PR as-is.

bachand · 2022-11-18T02:16:02Z

            }
        }
-        if let endOffset = endOffset, offsetInFile != endOffset {
+        if let endOffset = endOffset, offsetInFile < endOffset {


Since @jianjunwoo has been doing great thinking here and authored this code I'll also tag him here for visibility 👍

dfed added 2 commits November 4, 2022 17:22

Make ObjectiveCError type Sendable without warnings

189686f

bump podspec

ec01699

dfed requested review from bachand and fdiaz November 5, 2022 00:25

dfed added 2 commits November 9, 2022 18:38

Merge branch 'main' into dfed--warning-xcode-14

2f37d30

Simplify offsetInFile checking

5c9ec37

dfed commented Nov 10, 2022

View reviewed changes

Merge branch 'main' into dfed--warning-xcode-14

8e62c16

bachand approved these changes Nov 12, 2022

View reviewed changes

dfed commented Nov 17, 2022

View reviewed changes

Comment thread Sources/CacheAdvance/CacheReader.swift Outdated

Add comment

fd42fc0

bachand approved these changes Nov 18, 2022

View reviewed changes

Better comments and more complete checks

d051e71

dfed merged commit 3461cbb into main Nov 19, 2022

dfed deleted the dfed--warning-xcode-14 branch November 19, 2022 03:17

	} else if offsetInFile > endOffset {
	throw CacheAdvanceError.fileCorrupted
	}

Uh oh!

Conversation

dfed commented Nov 5, 2022

Uh oh!

codecov Bot commented Nov 5, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jianjunwoo Nov 19, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dfed commented Nov 10, 2022

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

codecov Bot commented Nov 5, 2022 •

edited

Loading

jianjunwoo Nov 19, 2022 •

edited

Loading