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

Use finish_with_state inside Rack::Logger #44454

Merged
merged 1 commit into from Feb 24, 2022

Conversation

jhawthorn
Copy link
Member

@jhawthorn jhawthorn commented Feb 17, 2022

This ensures that the finish instrumenter is sent to the same subscribers that the initial request was sent to.

This solves an issue where in a threaded environment the dynamic subscriptions from ActionDispatch::ServerTiming would cause a mismatch in the number of subscriptions to a topic, which would pop too many values off of the thread-local stacks, leading to invalid events being sent to subscribers.

I would like to backport this to 7-0-stable

Fixes #44167

cc @notapatch @jaynetics

This ensures that the finish instrumenter is sent to the same
subscribers that the initial request was sent to.

This solves an issue where in a threaded environment the dynamic
subscriptions from ActionDispatch::ServerTiming would cause a mismatch
in the number of subscriptions to a topic, which would pop too many
values off of the thread-local stacks, leading to invalid events being
sent to subscribers.

Fixes rails#44167
@jaynetics
Copy link
Contributor

i think this should prevent this specific event from triggering #44167 because it syncs the number of pushes to the shared stack with the number of pops.

however, it will not prevent other events from triggering the bug, e.g. it will recur if any commonly used gem calls #finish instead of #finish_with_state.

@notapatch
Copy link
Contributor

notapatch commented Feb 18, 2022

Hello @jhawthorn and @jaynetics

I'll be explicit about what I've done as I haven't done this before.

I took my reproduction application and changed Rails in the Gemfile to:

gem 'rails', github: 'rails/rails', ref: 'c8350cc7233411c9811303b8700185b4bfba9b80'

bundle update

The rails version in the Gemfile.lock says it's 7.1.0.alpha.

I then tried to reproduce the bug by stopping and starting the server a number of times and couldn't reproduce the bug.

I've updated the test application with the PR fix.

👍

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

Successfully merging this pull request may close these issues.

7.0.1: Activesupport instrumentor: nil can't be coerced into Float (TypeError)
3 participants