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

Further Puma 6 fix and fix for #26 #27

Merged
merged 1 commit into from
Nov 28, 2022
Merged

Conversation

ollym
Copy link
Contributor

@ollym ollym commented Nov 26, 2022

logger by default is events which doesn't work in Puma 6, it fails with: undefined method log' for #Puma::Events:0x00007f905c558720`

logger by default is `events` which doesn't work in Puma 6, it fails with: `undefined method `log' for #<Puma::Events:0x00007f905c558720>`
@ollym
Copy link
Contributor Author

ollym commented Nov 26, 2022

@Envek let me know if this is ok

@Envek
Copy link
Member

Envek commented Nov 27, 2022

But it should be overriden with correct value in create_server here:

logger = server.respond_to?(:log_writer) ? server.log_writer : events

So, most probably you should get undefined method 'log' for nil instead. Or am I missing something?

@ollym
Copy link
Contributor Author

ollym commented Nov 27, 2022

@Envek the value of logger before my PR defaults to events so if on_stopped fired before create_server then we're calling events.log which since Puma 6 was removed. This is the issue we're running into in production.

The other issue described in #26 is only possible because unless server&.shutting_down? does not guard if server is null, and if that's the case server.stop(true) will fail with undefined method 'stop' for nil.

Adjusting it to if server && !server.shutting_down? guards against server being null, which also guards against that block being called before create_server is called, so logger.log is guaranteed to work also.

Hope that makes more sense.

@Envek Envek merged commit 4f1a035 into yabeda-rb:master Nov 28, 2022
@Envek
Copy link
Member

Envek commented Nov 28, 2022

Makes total sense! Thank you!

Released in 0.7.1

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.

2 participants