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 systemd unix path handling #2007

Merged
merged 4 commits into from Oct 1, 2019

Conversation

MSP-Greg
Copy link
Member

@MSP-Greg MSP-Greg commented Oct 1, 2019

Puma should not unlink pre-existing UNIXSocket files. which causes issues with systemd. See: #1842, #1988

1st Commit: 'Fix Binder @unix_paths handling'

  1. Move removal to launcher from single and cluster.

  2. @unix_paths only contains files that Puma creates (pre-existing files are not)

2nd Commit: 'Add tests for pre-existing unix paths'

  1. test_binder.rb - test_pre_existing_unix

  2. test_integration_cluster.rb - test_pre_existing_unix

1. Move removal to launcher from single and cluster.

2. @unix_paths only contains files that Puma creates (pre-existing files are not)
1. test_binder.rb - test_pre_existing_unix

2. test_integration_cluster.rb - test_pre_existing_unix
This was referenced Oct 1, 2019
History.md Outdated
@@ -4,6 +4,7 @@
* Your feature goes here (#Github Number)

* Bugfixes
* Fix handling of pre-existing/systemd unix binder files (#1842, #1988)
Copy link
Member

Choose a reason for hiding this comment

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

let's mention "socket activation" and "systemd" explicitly for anyone scanning the changelog

Copy link
Member Author

Choose a reason for hiding this comment

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

Done

def test_pre_existing_unix
skip UNIX_SKT_MSG unless UNIX_SKT_EXIST

File.open(@bind_path, mode: 'wb') { |f| f.puts 'pre eixisting' }
Copy link
Member

Choose a reason for hiding this comment

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

existing

Copy link
Member Author

Choose a reason for hiding this comment

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

Done


File.open(@bind_path, mode: 'wb') { |f| f.puts 'pre eixisting' }

cli_server "-w #{WORKERS} -t 0:6 -q test/rackup/sleep_step.ru", unix: :unix
Copy link
Member

Choose a reason for hiding this comment

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

leave out the thread setting, not necessary

Copy link
Member Author

Choose a reason for hiding this comment

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

Done

skip UNIX_SKT_MSG unless UNIX_SKT_EXIST
unix_path = "test/#{name}_server.sock"

File.open(unix_path, mode: 'wb') { |f| f.puts 'pre eixisting' }
Copy link
Member

Choose a reason for hiding this comment

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

existing

Copy link
Member Author

Choose a reason for hiding this comment

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

Done

assert_match %r!unix://#{unix_path}!, @events.stdout.string

refute_includes @binder.instance_variable_get(:@unix_paths), unix_path

Copy link
Member

Choose a reason for hiding this comment

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

let's also call close_unix_paths and assert that the file is not removed

Copy link
Member Author

Choose a reason for hiding this comment

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

Done

@nateberkopec
Copy link
Member

@MSP-Greg Can you talk me through the changes to where @binder.close_unix_paths is called? Why was that changed? Is that just cleanup/refactor?

@nateberkopec nateberkopec added this to the 4.2.1 milestone Oct 1, 2019
@MSP-Greg
Copy link
Member Author

MSP-Greg commented Oct 1, 2019

@nateberkopec

changes to where @binder.close_unix_paths is called? Why was that changed? Is that just cleanup/refactor?

Ah, yeah, that's it. Or, I'm fixing my own mistake in a recent PR, as there's no need to have it in both cluster & single.

Six months from now, I don't want myself or someone else asking "why is that being done separately?"

@nateberkopec nateberkopec merged commit 17035b0 into puma:master Oct 1, 2019
@MSP-Greg MSP-Greg deleted the fix-systemd-unix-paths branch September 20, 2020 14:04
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