Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix 3235 #3316

Merged
merged 2 commits into from
Dec 19, 2023
Merged

Fix 3235 #3316

merged 2 commits into from
Dec 19, 2023

Conversation

kyay10
Copy link
Collaborator

@kyay10 kyay10 commented Dec 8, 2023

fixes #3235. See that issue for more information

@nomisRev
Copy link
Member

nomisRev commented Dec 9, 2023

@kyay10 RaiseCancellationException is still referenced in the e.raisedOrRethrow. It's safe there because it's not inlined, and thus called from our module? 🤔

@kyay10
Copy link
Collaborator Author

kyay10 commented Dec 9, 2023

Exactly. It's fine there, just not when inlined. In fact, the reason this wasn't caught earlier is because fold doesn't catch RaiseCancellationException, it catches CancellationException. traced is the only inline method that deals with RaiseCancellationException directly, and I think simply no one has really tried out traced outside of this repo, hence why no one ran into this before.

@serras serras requested a review from nomisRev December 11, 2023 14:16
@serras
Copy link
Member

serras commented Dec 11, 2023

This makes sense to me. One question, though, @kyay10: why this change and not making RaiseCancellationException @PublishedApi internal?

@kyay10
Copy link
Collaborator Author

kyay10 commented Dec 11, 2023

@serras I just figured making it part of the API wasn't really needed. Making RaiseCancellationException published would work in the same way. Note, btw, that fold also simply catches CancellationException and relies on raisedOrRethrow to determine whether the exception belongs to the current raise, so this change matches fold more closely

@serras
Copy link
Member

serras commented Dec 12, 2023

@kyay10 thanks for the info! I agree that it makes sense to mirror fold.

@serras serras merged commit 3bce67e into arrow-kt:main Dec 19, 2023
10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

IllegalAccessError when using Raise
3 participants