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

Core: Fix crash when trying to log an exception #4313

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

NewSoupVi
Copy link
Member

@NewSoupVi NewSoupVi commented Dec 1, 2024

In #3028, we added a new logging filter which checked record.msg.

However, you can pass whatever you want into a logging call. In this case, what we missed was https://github.com/ArchipelagoMW/Archipelago/blob/ecc3094c70b3ee1f3e18d9299c03198564ec261a/MultiServer.py#L530C1-L530C37, where we pass an Exception object as the message. This currently causes a crash with the new filter.

As far as I can tell, the logging module supports this. It has no typing and can handle passing objects as messages just fine, it just converts them to a string.

What you're supposed to use, as far as I understand it, is record.getMessage() instead of record.msg, which is guaranteed to be a string that represents what the final message will actually be.

In #3028, we added a new logging filter which checked `record.msg`. 

However, you can pass whatever you want into a logging call. In this case, what we missed was https://github.com/ArchipelagoMW/Archipelago/blob/ecc3094c70b3ee1f3e18d9299c03198564ec261a/MultiServer.py#L530C1-L530C37, where we pass an Exception object as the message. This currently causes a crash with the new filter.

The logging module supports this. It has no typing and can handle passing objects as messages just fine.

What you're supposed to use, as far as I understand it, is `record.getMessage()` instead of `record.msg`.
@github-actions github-actions bot added the affects: core Issues/PRs that touch core and may need additional validation. label Dec 1, 2024
@NewSoupVi
Copy link
Member Author

NewSoupVi commented Dec 1, 2024

If this isn't just merged by a different core maintainer without peer review (prayge), then @kedNalatacId would like your peer review on this if you have a minute to look at it

@NewSoupVi NewSoupVi added is: bug/fix Issues that are reporting bugs or pull requests that are fixing bugs. affects: release/blocker Issues/PRs that must be addressed before next official release. waiting-on: peer-review Issue/PR has not been reviewed by enough people yet. labels Dec 1, 2024
@black-sliver
Copy link
Member

black-sliver commented Dec 1, 2024

@Berserker66 you think that could've crashed MultiHoster11 ?
No, _save is the impl that does not run on WebHost.
This should not be in active_webhost (see Vi's comment below)

A similar line exists in customserver here:

logger.exception(e)

@NewSoupVi
Copy link
Member Author

NewSoupVi commented Dec 1, 2024

The PR that added this was merged two days ago

@kedNalatacId
Copy link
Contributor

If this isn't just merged by a different core maintainer without peer review (prayge), then @kedNalatacId would like your peer review on this if you have a minute to look at it

looks good to me; sorry for missing this use case.

Utils.py Outdated Show resolved Hide resolved
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
affects: core Issues/PRs that touch core and may need additional validation. affects: release/blocker Issues/PRs that must be addressed before next official release. is: bug/fix Issues that are reporting bugs or pull requests that are fixing bugs. waiting-on: peer-review Issue/PR has not been reviewed by enough people yet.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants