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

Fix races in publish_subscribe_test.rb and update truffleruby #1239

Merged
merged 2 commits into from Nov 17, 2023

Conversation

eregon
Copy link
Contributor

@eregon eregon commented Nov 17, 2023

* Older versions are not supported.
@byroot
Copy link
Collaborator

byroot commented Nov 17, 2023

You angered rubocop, but otherwise LGTM.

@eregon
Copy link
Contributor Author

eregon commented Nov 17, 2023

Fixed, and the CI passed except for that rubocop complaint: https://github.com/redis/redis-rb/actions/runs/6904372003/job/18784907013?pr=1239

@eregon
Copy link
Contributor Author

eregon commented Nov 17, 2023

https://github.com/redis/redis-rb/actions/runs/6904417499/job/18785060895?pr=1239

  1) Failure:
TestPublishSubscribe#test_subscribe_from_another_thread [/home/runner/work/redis-rb/redis-rb/test/redis/publish_subscribe_test.rb:332]:
Expected nil to not be nil.

This is unrelated to this PR, it's doing refute_nil thread.join(2).

Thread#join with a duration is basically an anti-pattern, even more so in tests.
I'll push a commit removing the duration, time can vary a lot in CI, so a duration is just asking for more transients.

@byroot
Copy link
Collaborator

byroot commented Nov 17, 2023

Thread#join with a duration is basically an anti-pattern, even more so in tests.

The reason is to avoid locking forever, if the thread is stuck trying to read on a socket or some other blocking operation, your CI runner hang for long enough that it's just killed and you have 0 debug information.

I'm not against bumping it, but I wouldn't remove it.

@eregon
Copy link
Contributor Author

eregon commented Nov 17, 2023

If running locally, one would Ctrl+C and then get a stacktrace, so there better without a duration as well.

Yeah, GitHub Actions doesn't send SIGINT on timeout I guess, unfortunate.
IMO it should be the job of the test harness to print thread stracktraces if e.g. a test takes longer than a large timeout (e.g. some minutes).
Note the failure above gives very little information.

@byroot
Copy link
Collaborator

byroot commented Nov 17, 2023

IMO it should be the job of the test harness to print thread stracktraces

Sure, but in the meantime minitest doesn't offer that, so until it does, a generous .join(5) is prefereable to .join.

Note the failure above gives very little information.

They at least give you which test failed, which is a decent start.

@eregon
Copy link
Contributor Author

eregon commented Nov 17, 2023

BTW there are many other cases of thread.join in the same file, so I'd suggest to try it like this, and if a test is hanging transiently and cannot be reproduced easily locally then either modify that one to e.g. show thread.backtrace if not joined in N seconds, or improve the test harness (or in test helpers maybe) to do that automatically.

@eregon
Copy link
Contributor Author

eregon commented Nov 17, 2023

As you wish, should I remove the last commit from this PR or keep it?

@byroot
Copy link
Collaborator

byroot commented Nov 17, 2023

Please remove it yes.

@eregon
Copy link
Contributor Author

eregon commented Nov 17, 2023

OK.

They at least give you which test failed, which is a decent start.

That's also shown if the test hangs, because the test name is printed before it starts running.

@eregon
Copy link
Contributor Author

eregon commented Nov 17, 2023

Removed, and ready to merge.

@byroot byroot merged commit 58e43cf into redis:master Nov 17, 2023
32 of 34 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.

None yet

2 participants