Skip to content

Halt with mutable backtrace - #116

Merged
flash-gordon merged 1 commit into
dry-rb:masterfrom
johnmaxwell:master
Dec 10, 2019
Merged

Halt with mutable backtrace#116
flash-gordon merged 1 commit into
dry-rb:masterfrom
johnmaxwell:master

Conversation

@johnmaxwell

Copy link
Copy Markdown
Contributor

Exceptions are typically raised with mutable backtraces. This fixes an
issue with spring where all backtraces are sanitized to remove spring
references. #115

Exceptions are typically raised with mutable backtraces. This fixes an
issue with spring where all backtraces are sanitized to remove spring
references.
@flash-gordon

Copy link
Copy Markdown
Member

I can counter that with typically libraries don't patch the kernel methods, eh? Anyway, thank you for working on this. Do you think we could reuse the same mutable array for all backtraces? I hope rails doesn't add lines to them?

@johnmaxwell

johnmaxwell commented Dec 10, 2019

Copy link
Copy Markdown
Contributor Author

🤚Hey, no problem. 🤚I'm just trying to clarify intent in the commit message -- not be argumentative. I get that dry-monads is just one half of this incompatibility, and I agree that patching Kernel is a bold move.

I opened tickets on both projects. You responded and they haven't, so I just wanted to assist by opening this PR. I appreciate your work.

Do you think we could reuse the same mutable array for all backtraces? I hope rails doesn't add lines to them?

I probably would not share the empty backtrace arrays knowing what we know, but I can tell you are being careful with allocations. To my knowledge Rails doesn't add stuff to backtraces, but we've been surprised once already :)

Side note: is this use-case ripe for throw/catch, since this Halt exception is part of routine function and not bound for the outside world? 🤷🏻‍♂️

Ruby’s throw and catch are not used for exceptions. Instead, they provide a way to terminate execution early when no further work is needed
-- http://rubylearning.com/blog/2011/07/12/throw-catch-raise-rescue--im-so-confused/

EDIT: Nevermind, I see this now: #98 (comment)

@flash-gordon

Copy link
Copy Markdown
Member

Sure, my reply wasn't personal at all. It's like rails never cease to wonder.

I probably would not share the empty backtrace arrays knowing what we know, but I can tell you are being careful with allocations. To my knowledge Rails doesn't add stuff to backtraces, but we've been surprised once already :)

Right, let's just ship it. I'll push a release tomorrow.

Side note: is this use-case ripe for throw/catch, since this Halt exception is part of routine function and not bound for the outside world? 🤷🏻‍♂️

It's local and it can be done with a simple return statement. However, the goal is to make transaction blocks in do notation aware of interruption, see https://dry-rb.org/gems/dry-monads/1.0/do-notation/#transaction-safety

@flash-gordon
flash-gordon merged commit 41a3d39 into dry-rb:master Dec 10, 2019
@johnmaxwell

Copy link
Copy Markdown
Contributor Author

Thank you!

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.

2 participants