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

Debug print loaded extensions if PUMA_DEBUG is set #3030

Closed

Conversation

jeremyevans
Copy link
Contributor

Description

This implements the feature requested in #3020, printing the loaded extensions if PUMA_DEBUG is set.

Not sure if this is implemented in the best place, but trying to move it to a separate method seemed challenging as the existing method appears to be called for multiple places, and I wanted to avoid redundancy.

Your checklist for this pull request

  • I have reviewed the guidelines for contributing to this repository.
  • I have added (or updated) appropriate tests if this PR fixes a bug or adds a feature.
    I didn't do this because there are no existing tests for printing the listening socket.
  • My pull request is 100 lines added/removed or less so that it can be easily reviewed.
  • If this PR doesn't need tests (docs change), I added [ci skip] to the title of the PR.
  • If this closes any issues, I have added "Closes #issue" to the PR description or my commit messages.
  • I have updated the documentation accordingly.
    I didn't do this as there is no existing documentation for PUMA_DEBUG, beyond a comment in the systemd doc.
  • All new and existing tests passed, including Rubocop.
    CI should be able to check this. I didn't run the tests nor Rubocop locally, though all CI jobs that ran in my fork passed.

Not sure if this is the best place for it, but trying to move it
to a separate method seemed challenging as this appears to be
called for multiple places.

Closes puma#3020
This needs to be anchored to the end of the string.
@MSP-Greg MSP-Greg force-pushed the debug-print-loaded-extensions-3020 branch from a905543 to 62c552a Compare December 11, 2022 04:24
@MSP-Greg
Copy link
Member

@jeremyevans

Thank you for the PR.

Not sure if this is implemented in the best place

That's the messy part. Placing the code in Binder#parse has some issues.

If one starts Puma with a control server using a socket connection, the control server calls Binder#parse for its bind address, so the loaded extensions will be logged twice.

If one is using a 'single' setup (with no workers), one can log from the main process, but if one is using workers (a clustered setup), it would probably be best to log from Worker index 0 (Worker0). As an example, when running clustered, nio is loaded in the Worker0 process, it is not loaded in the main process.

I've got a patch for that, not sure about how often/when to log the Worker0 extensions, but it does log correctly when the server starts.

Also, the code below is using file extensions for system libraries for macOS & Windows, which are different from the file extensions used by Ruby extensions (.bundle & .so, respectively).

`$LOADED_FEATURES.grep(/\.(so|dylib|dll)\z/i).each { |f| log_writer.debug("    #{f}") }`

So, in the patch I've got I used:

re_ext = /\.#{RbConfig::CONFIG['DLEXT']}\z/i
$LOADED_FEATURES.grep(re_ext).each { |f| @log_writer.debug("  #{f}") }

Lastly, I have yet to try any of the code with non-MRI Rubies, not sure what they'll do. Additionally, two items, first, when and how to log Worker0 on restarts, and second, I can't recall if $LOADED_FEATURES resolves symlinks or not. People may prefer to see resolved (actual) paths.

I can push the patch if you'd like... Again, thanks.

Not sure if this is the best place for it, but trying to move it
to a separate method seemed challenging as this appears to be
called for multiple places.

Closes puma#3020
This needs to be anchored to the end of the string.
@jeremyevans
Copy link
Contributor Author

@MSP-Greg Please do push your patch, it's definitely better than what I submitted. $LOADED_FEATURES will generally store real paths (which resolves symlinks).

@MSP-Greg
Copy link
Member

@jeremyevans

Pushed the commit, the CI error is one of those intermittent issues that we have. We normally squash PR commits, I always have 'merge' commits when rebasing PR branches. Must be doing something wrong...

@MSP-Greg
Copy link
Member

@jeremyevans I made a mess of the branch. Can't seem to force push to the branch.

I think the last commit Merge branch 'debug-print-loaded-extensions-3020... and the first set of commits (Debug print loaded extensions if PUMA_DEBUG is set and Fix regexp used for determining loading extensions) need to be deleted? I hate it when this happens. Sorry.

@jeremyevans
Copy link
Contributor Author

No big deal. I think the best approach here is to delete this PR, and for you to push a PR with your changes. Does that make sense?

@MSP-Greg
Copy link
Member

Again, sorry for the mess in your branch. I created PR #3036.

@MSP-Greg MSP-Greg closed this Dec 14, 2022
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