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

CI: Use Tempfile.create in TestIntegration #3410

Merged
merged 1 commit into from
Jun 9, 2024

Conversation

dentarg
Copy link
Member

@dentarg dentarg commented Jun 9, 2024

@dentarg dentarg force-pushed the tempfile/follow-up-to-3408 branch from c97364c to dae34ff Compare June 9, 2024 14:57
@ioquatix
Copy link
Contributor

ioquatix commented Jun 9, 2024

The reason why I assigned it to an instance variable was to prevent GC. So I'm not sure why this change is required, however, I'm also not sure my change fixed the issue, since we saw some other IO related issue... I need to spend more time investigating this too.

Copy link
Member

@MSP-Greg MSP-Greg left a comment

Choose a reason for hiding this comment

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

Maybe:

if @config_file&.exist?
  @config_file.unlink
  @config_file = nil
end

@MSP-Greg
Copy link
Member

MSP-Greg commented Jun 9, 2024

The reason why I assigned it to an instance variable was to prevent GC.

Agree, that is an issue. A lot of these issues may be related to running test parallel, along with 'resource starved' CI runners. In the test update (which I haven't looked at for a while), I think I removed all the Tempfile.new use, along with a lot of fixes for blocking IO, etc, It was a lot more stable. There were a few lib file changes, so I started on extracing them, but haven't finished...

Re Tempfile.create, since we know that unlink will always run, why create finalizers?

@ioquatix
Copy link
Contributor

ioquatix commented Jun 9, 2024

Re Tempfile.create, since we know that unlink will always run, why create finalizers?

The only reason would be if you think there is a code path where that unlink is not called. e.g. non-trivial flow controls like exceptions, etc might skip your explicit unlink.

@dentarg
Copy link
Member Author

dentarg commented Jun 9, 2024

All tests here are green so let's merge this and see how it plays out going forward?

@MSP-Greg
Copy link
Member

MSP-Greg commented Jun 9, 2024

@ioquatix I suspect if something stopped teardown, it's pretty severe, like a SEGV...

@dentarg dentarg merged commit 5de7ccd into puma:master Jun 9, 2024
74 checks passed
@dentarg dentarg deleted the tempfile/follow-up-to-3408 branch June 9, 2024 18:46
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.

None yet

3 participants