Skip to content

Comments

std.logger.core unittest forgotton thread.join#8557

Merged
dlang-bot merged 1 commit intodlang:masterfrom
burner:std.logger_32_race_fix
Sep 4, 2022
Merged

std.logger.core unittest forgotton thread.join#8557
dlang-bot merged 1 commit intodlang:masterfrom
burner:std.logger_32_race_fix

Conversation

@burner
Copy link
Member

@burner burner commented Sep 4, 2022

At this point I'm quite sure that the sporadic unittest errors happen because the created Thread is still active after the unittest is finish. The log message is written after the test is done.

This PR makes the test join the thread.

@dlang-bot
Copy link
Contributor

Thanks for your pull request, @burner!

Bugzilla references

Your PR doesn't reference any Bugzilla issue.

If your PR contains non-trivial changes, please reference a Bugzilla issue or create a manual changelog.

Testing this PR locally

If you don't have a local development environment setup, you can use Digger to test this PR:

dub run digger -- build "master + phobos#8557"

@burner
Copy link
Member Author

burner commented Sep 4, 2022

@pbackus @Geod24 FYI

@pbackus
Copy link
Contributor

pbackus commented Sep 4, 2022

Thanks!

@burner burner mentioned this pull request Sep 4, 2022
@pbackus
Copy link
Contributor

pbackus commented Sep 4, 2022

Buildkite appears to be stuck on sociomantic-tsunami/ocean after reporting

Exited with status -1 (agent lost)

Maybe try re-running CI?


() @trusted
{
Thread.sleep(dur!"msecs"(10));
Copy link
Member

Choose a reason for hiding this comment

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

Why a sleep is required?

Copy link
Member Author

Choose a reason for hiding this comment

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

It actually might not be required.

Thanks

Copy link
Member

Choose a reason for hiding this comment

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

I think de should annotate the join as @safe, if we can. Just a note

Copy link
Contributor

Choose a reason for hiding this comment

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

Unfortunately it is a virtual method, so adding @safe could break code that overrides it.

@burner burner force-pushed the std.logger_32_race_fix branch from c66f3c8 to ebde108 Compare September 4, 2022 21:04
@dlang-bot dlang-bot merged commit 34ad214 into dlang:master Sep 4, 2022
@burner
Copy link
Member Author

burner commented Sep 5, 2022

thank you all

@atilaneves
Copy link
Contributor

Does this fix https://issues.dlang.org/show_bug.cgi?id=23286 ?

@burner
Copy link
Member Author

burner commented Sep 6, 2022

Does this fix https://issues.dlang.org/show_bug.cgi?id=23286 ?

As I can't reproduce this bug, Idk.

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.

7 participants