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

Next steps for 4.2.1 #1998

Closed
nateberkopec opened this issue Sep 26, 2019 · 7 comments
Closed

Next steps for 4.2.1 #1998

nateberkopec opened this issue Sep 26, 2019 · 7 comments
Labels

Comments

@nateberkopec
Copy link
Member

nateberkopec commented Sep 26, 2019

#1988 (comment) moving disco with @MSP-Greg here to keep stuff on-topic on the bug report thread

Puma has always worked fine if the UNIXSocket files were not removed, so I think we should remove all code that does so. Maybe even test for that

This annoys me, but it might be true. It annoys me because if we create a file, we should clean it up when we don't need it anymore. I'll have to look at the original issue more to reflect. So, gut feeling, I don't like this course of action.

I think there are some duplicate test method names, so maybe we should use file names that also include the test class...

Dunno what you mean.

Maybe get this, #1986, and whatever else patched, do a point release, let the dust settle, and start back on the refactoring work?

Yup. I think the socket-closed bug + #1986 is a good point release.

Also, maybe set JRuby back to 'allow-failure' until it can be made stable? After all the work on tests, always having to check logs, etc, I'd really like to see a few green checks...

NO, NO GREEN CHECKS FOR YOU! </soup-nazi> Forgetting about JRuby dug us into a hole the last time we did this. For now, I want to feel the pain of JRuby constantly failing, so that when 4.2.1 is released JRuby is the first thing we want to go and fix.

If you really really want to we can allow_fail until 4.2.1 is released, but I'm going to add it back in as soon as that release is tagged.

@mcg
Copy link

mcg commented Sep 26, 2019

Puma has always worked fine if the UNIXSocket files were not removed, so I think we should remove all code that does so. Maybe even test for that

This annoys me, but it might be true. It annoys me because if we create a file, we should clean it up when we don't need it anymore. I'll have to look at the original issue more to reflect. So, gut feeling, I don't like this course of action.

Just to clarify, in the case of systemd socket activation, systemd initially creates the socket file, not Puma.

@MSP-Greg
Copy link
Member

MSP-Greg commented Sep 26, 2019

@mcg

systemd initially creates the socket file

Is that true before the first start of Puma?

If so, maybe what we need to do is when Puma starts, check for whether the file(s) exist, and set a flag that is used to determine whether to unlink the file(s) on shutdown?

Or, just not add them to the Binder @unix_paths array if they do exist?

@mcg
Copy link

mcg commented Sep 26, 2019

@MSP-Greg Correct. systemd socket unit, creates the socket(file or port).

@MSP-Greg
Copy link
Member

@nateberkopec

Sorry, I've got a really squirrely ruby master bug that I'm trying to understand, just happened with a build early this morning...

Dunno what you mean.

Sorry, being stupid. Deleting the UNIXSocket files in Puma is different than deleting the files in test teardown.

Re JRuby, I've tried it with Actions (both 9.2.8.0 & head), and they freeze all the time, but sometimes have only a few failures/errors. Until they stop freezing, I don't think I can be of any help, but I'll certainly watch for the fix.

@MSP-Greg
Copy link
Member

I believe #1987 solves the original bug.

#1992 needs to be modified to work with systemd, as it was done as a 'please try this' PR.

Puma currently unlinks all binder UNIXSocket files on shutdown or (hard/exec) restart.

Puma should only unlink UNIXSocket files that it creates, and leave the files created by systemd alone. We can change the Binder @unix_paths array to only include files created by Puma.

I think the changes will cause merge conflicts with #1987, or #1987 blocks #1992.

@nateberkopec
Copy link
Member Author

Puma should only unlink UNIXSocket files that it creates, and leave the files created by systemd alone.

Got it, definitely agree with this.

@nateberkopec
Copy link
Member Author

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants