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

De-duplicate error spam. #84724

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

lawnjelly
Copy link
Member

@lawnjelly lawnjelly commented Nov 10, 2023

Prevent large amounts of duplicate error spam reaching the MessageQueue.

Helps address #84713 .

Notes

  • Simple solution for error spam within the same frame for discussion.
  • Compiled out when compiled with TESTS_ENABLED, because tests rely on duplicate errors being individually reported.
  • Could alternatively runtime check whether the tests are being run.
  • Toaster might deal with duplicates, but is preferable in some ways to prevent this before spam reaches the MessageQueue, so not to fill it up.
  • This won't deal with duplication of 2 or more error messages.
  • Could possibly pass the number of duplicates in the error message if desired (with some cunning), perhaps to the toaster so it could correctly report number of duplicates without straining MessageQueue.
  • Uses mutex just in case of multithread calls to _err_print_error(). Could possibly just lock around the whole function.

@lawnjelly lawnjelly added this to the 4.x milestone Nov 10, 2023
@lawnjelly lawnjelly marked this pull request as ready for review November 10, 2023 16:35
@lawnjelly lawnjelly requested a review from a team as a code owner November 10, 2023 16:35
@Calinou
Copy link
Member

Calinou commented Nov 10, 2023

This makes sense, but I suggest documenting this in the ERR_ and WARN_ macros that if you want several errors from the same function in the same frame to be printed, they must differentiate themselves in some way (e.g. by inserting some context about function parameters, etc). This is good practice in general.

Also consider that the editor Output panel can collapse lines already, although this is disabled by default: #84499

@lawnjelly
Copy link
Member Author

lawnjelly commented Nov 10, 2023

This makes sense, but I suggest documenting this in the ERR_ and WARN_ macros that if you want several errors from the same function in the same frame to be printed, they must differentiate themselves in some way (e.g. by inserting some context).

Good idea I'll do this. 👍
(Although might be subject to change if we think of variations of how to do the de-duplication.)

Also consider that the editor Output panel can collapse lines already, although this is disabled by default: #84499

Yes, the problem here is though that that doesn't prevent the spam entering the MessageQueue, which can fill it up and lock up the editor.

UPDATE:
Ah see asan is failing. Might be to do with the static lifetime of CharString and COW etc. I'll have a look in the morning.

@lawnjelly
Copy link
Member Author

lawnjelly commented Nov 11, 2023

Intriguingly, the tests failing seems not to be to do with the static lifetimes or lack of mutex, they are failing because l->errfunc() is not called in the case of a duplicate.

Maybe the tests are counting the number of errors or something... Still investigating.

Yes:

	SUBCASE("set_type_variation") {
		for (const StringName &name : names) {
			Ref<Theme> theme = memnew(Theme);

			ErrorDetector ed;
			theme->set_type_variation(valid_type_name, name);
			CHECK(ed.has_error);
		}

Fails here, as there is no error, as the test is presumably duplicating the previous error. Maybe the solution is simple - to turn off deduplication during tests. 🤷‍♂️

@lawnjelly lawnjelly force-pushed the dedup_error_spam branch 2 times, most recently from 779ee25 to b6d0c2b Compare November 11, 2023 06:02
Prevent large amounts of duplicate error spam reaching the `MessageQueue`.
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.

3 participants