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

Ensure That Metrics Only Decrements Active Requests Once #2916

Merged

Conversation

isomarcte
Copy link
Member

@isomarcte isomarcte commented Oct 16, 2019

Prior to this commit there was an edge case where the ops.decreaseActiveRequests would get run more than once if an error or cancellation occurred after the onFinalize method was run in onResponse.

This change adds a Ref which is initialized at the start of request handling and set to true once the ops.decreaseActiveRequests is run. All attempts to set ops.decreaseActiveRequests are guarded by this boolean value.

@isomarcte
Copy link
Member Author

@isomarcte isomarcte commented Oct 16, 2019

This may not be a bug. I'm actually not 100% sure anymore. Leaving open until I can think on it some more.

@isomarcte
Copy link
Member Author

@isomarcte isomarcte commented Oct 17, 2019

After some additional testing, I do still believe there is an edge case here and I do still think the changes I've proposed will fix it.

Here is what I know,

  • I currently have a http4s 0.20.11 server running with prometheus based metrics
  • Currently, the org_http4s_server_active_request value is at -1.0.

Here is what I suspect.
If cancellation or error occurs in the onEmpty handler, it is possible that the code to decrease the active request count will run twice, once in the onEmpty handler and once in either of the non-success case branches of the bracketCase release handler. This is because there is no synchronization on decrementing that value.

So, my brain is exhausted. I'll update the commit/PR notes tomorrow with some better, and hopefully actually correct, examples if not tests cases.

Prior to this commit there was an edge case where the `ops.decreaseActiveRequests` would get run more than once if an error or cancellation occurred _after_ the `onFinalize` method was run in `onResponse`.

This change adds a `Ref` which is initialized at the start of request handling and set to `true` once the `ops.decreaseActiveRequests` is run. All attempts to set `ops.decreaseActiveRequests` are guarded by this boolean value.
@isomarcte isomarcte force-pushed the ensure-metrics-decrement-at-most-once branch from 18e6d37 to 91944e9 Compare Oct 31, 2019
Copy link
Member

@ChristopherDavenport ChristopherDavenport left a comment

Bit of a hammer, does add a little weight for each request, but I think its worth it to outlaw this condition.

@rossabaker rossabaker merged commit 0047baf into http4s:master Nov 26, 2019
1 of 2 checks passed
@rossabaker
Copy link
Member

@rossabaker rossabaker commented Nov 26, 2019

Oops. This is a bugfix. I should have backported this one.

rossabaker pushed a commit that referenced this issue Nov 26, 2019
Prior to this commit there was an edge case where the `ops.decreaseActiveRequests` would get run more than once if an error or cancellation occurred _after_ the `onFinalize` method was run in `onResponse`.

This change adds a `Ref` which is initialized at the start of request handling and set to `true` once the `ops.decreaseActiveRequests` is run. All attempts to set `ops.decreaseActiveRequests` are guarded by this boolean value.
rossabaker pushed a commit that referenced this issue Nov 26, 2019
Prior to this commit there was an edge case where the `ops.decreaseActiveRequests` would get run more than once if an error or cancellation occurred _after_ the `onFinalize` method was run in `onResponse`.

This change adds a `Ref` which is initialized at the start of request handling and set to `true` once the `ops.decreaseActiveRequests` is run. All attempts to set `ops.decreaseActiveRequests` are guarded by this boolean value.
@isomarcte isomarcte deleted the ensure-metrics-decrement-at-most-once branch Dec 3, 2019
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

3 participants