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

Prevent logging exceptions twice when debugger is opened #15746

Merged
merged 1 commit into from
Dec 19, 2023

Conversation

hernanmd
Copy link
Member

@hernanmd hernanmd commented Dec 8, 2023

Change logDebuggerStackToFile default setting to false since when set to true, it makes the debug session log twice on every request as reported in #15620.

…ng was making the debug session log twice on every request.
@guillep
Copy link
Member

guillep commented Dec 11, 2023

Maybe you would like to discuss this with @StevenCostiou ?

If Pharo is already logging exceptions, why should the debugger do it also?
My point here is that maybe the code of this setting should just be removed, but I'm not sure :).
Otherwise, changing the setting looks like it's not solving the cause of the problem but its consequence, right? People with a bad setting could also have a bad experience, and not understand where it comes from.
In other words: if the setting has no value, why having it?

@StevenCostiou
Copy link
Collaborator

After discussion, we propose to merge this PR and open an issue to specifically discuss the need for a second mechanism.
It is not clear right now if we do or not.
However not logging upon debugger opening can improve a bit the speed? - and we need to gain avery possible bit of speed in the debugger ;)

@hernanmd
Copy link
Member Author

Ok, I created another issue for this: #15810

@Ducasse Ducasse merged commit 0c5f60d into pharo-project:Pharo12 Dec 19, 2023
@Ducasse
Copy link
Member

Ducasse commented Dec 19, 2023

I merge as requeted.

@daniels220
Copy link
Contributor

Dang, I've been thinking about saying something about this for a long time, just now getting to it—wish I'd slipped my thoughts in sooner, because:

I think, instead, the system should skip the unconditional logging to the file, in UIManager>>requestDebuggerOpeningFor:, allowing the existing setting to do exactly what it says—control whether the stack is logged at all when opening a debugger. In which case I think true is a sensible default. As a fallback we should also log in requestDebuggerOpeningFor: if OupsDebuggerSystem is not present.

@guillep
Copy link
Member

guillep commented Dec 20, 2023

Dang, I've been thinking about saying something about this for a long time, just now getting to it—wish I'd slipped my thoughts in sooner, because:

I think, instead, the system should skip the unconditional logging to the file, in UIManager>>requestDebuggerOpeningFor:, allowing the existing setting to do exactly what it says—control whether the stack is logged at all when opening a debugger. In which case I think true is a sensible default. As a fallback we should also log in requestDebuggerOpeningFor: if OupsDebuggerSystem is not present.

I like the idea :)

@daniels220
Copy link
Contributor

Actually, I'm going to kinda take that back—but I'm also going to put my actual comment on #15810, since that's the open issue.

@hernanmd hernanmd deleted the PharoDebug_logs_error_twice branch June 18, 2024 10:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants