Skip to content

Adjust print_response to separate error-printing from result display, remove loop #59218

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

Merged
merged 10 commits into from
Aug 12, 2025

Conversation

BioTurboNick
Copy link
Contributor

@BioTurboNick BioTurboNick commented Aug 6, 2025

Took a stab at fixing #59209 , but it could be introducing other ways it could fail that I'm not appreciating?

1.11.4 logic:

  1. If it's an error, print the error then break and return
  2. If it's an error, and printing the error causes an error, catch it, try two levels of fallback printing, then break and return
  3. If it's okay, display the result then break and return
  4. If it's okay, and displaying the result causes an error, print a message, rethrow, catch, populate the response variables with the new exception info, and start the loop over to follow the error printing pathway for those values

In no case does the loop execute more than twice.

In 1.11.5+, condition 4 ends up following path 2, and path 4 would only be followed if there's an exception in the scaffolding around the display invocations.

I restructured it to take out the loop, now it just attempts display if there was no error, accounts for errors during display, and then errors from either response or display are printed.

I'm not sure if I used the sigatomic_end calls correctly. EDIT: See comment below

Also tweaked the final fallback message because it suggests the issue is that the exception per se is a nested type, rather than that printing one exception is triggering another exception. And added a condition to trim REPL frames when it originates from within print_response.

Fixes #59209

@BioTurboNick BioTurboNick changed the title Adjust print_response loop to separate error-printing from result display Adjust print_response to separate error-printing from result display, remove loop Aug 6, 2025
@BioTurboNick
Copy link
Contributor Author

BioTurboNick commented Aug 6, 2025

The 1 CI failure seemed to be an unrelated timeout or similar, trying to merge/rerun one time before marking for review....

EDIT: All green

@BioTurboNick BioTurboNick marked this pull request as ready for review August 6, 2025 19:10
@BioTurboNick
Copy link
Contributor Author

I think I understand sigatomic_ now. The bracketing _begin and _end place everything in the function into an uninterruptable state so that the primary functions of the REPL happen in an orderly fashion. However, we want to temporarily allow interruption during potentially long-running code, like display of a result or processing/printing a stacktrace. So the atomic region is ended just before those sections, and either restored after by a _begin or invisibly restored by the try block ending. Everything in the catch block operates in the sigatomic context of the enclosing block.

@BioTurboNick
Copy link
Contributor Author

This would be good for 1.12 backport. Could backport to 1.11 too, though may be a judgment call.

@oscardssmith oscardssmith requested a review from vtjnash August 12, 2025 16:53
Copy link
Member

@vtjnash vtjnash left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the Base.sigatomic_end stuff is some legacy baggage that keeps getting copied here by accident, but the change seems to look right (and work right on my machine in brief testing)

@oscardssmith oscardssmith merged commit 50c9d21 into JuliaLang:master Aug 12, 2025
7 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.

SYSTEM (REPL): showing an error caused an error when error occurs in display path (1.11.5+)
3 participants