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: ensure we don't send multiple flag requests in parallel #423

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions posthog-node/CHANGELOG.md
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
# Next

1. Fix: prevent fetch floods when rate-limited.
2. Fix: never send multiple feature flag fetch requests in parallel.

# 4.10.1 – 2025-03-06

Expand Down
13 changes: 11 additions & 2 deletions posthog-node/src/feature-flags.ts
Original file line number Diff line number Diff line change
Expand Up @@ -81,6 +81,7 @@ class FeatureFlagsPoller {
this.projectApiKey = projectApiKey
this.host = host
this.poller = undefined
this.inflightRequest = undefined
Copy link
Contributor

Choose a reason for hiding this comment

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

syntax: Missing type declaration for inflightRequest class property

Suggested change
this.inflightRequest = undefined
inflightRequest?: Promise<PostHogFetchResponse>
this.inflightRequest = undefined

this.fetch = options.fetch || fetch
this.onError = options.onError
this.customHeaders = customHeaders
Expand Down Expand Up @@ -396,11 +397,19 @@ class FeatureFlagsPoller {
clearTimeout(this.poller)
this.poller = undefined
}

this.poller = setTimeout(() => this._loadFeatureFlags(), this.getPollingInterval())

try {
const res = await this._requestFeatureFlagDefinitions()
// guard against loading flags multiple times in concurrently.
// there's no point in starting a new fetch if there's already one inflight
if (this.inflightRequest) {
// wait for the request to finish so we don't return empty flags
await this.inflightRequest
return
}
this.inflightRequest = this._requestFeatureFlagDefinitions()
const res = await this.inflightRequest
this.inflightRequest = undefined
Comment on lines +411 to +412
Copy link
Contributor

Choose a reason for hiding this comment

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

style: Consider adding error handling around the await to ensure inflightRequest is always cleared, even if the request fails

Suggested change
const res = await this.inflightRequest
this.inflightRequest = undefined
try {
const res = await this.inflightRequest
return res
} finally {
this.inflightRequest = undefined
}


// Handle undefined res case, this shouldn't happen, but it doesn't hurt to handle it anyway
if (!res) {
Expand Down
Loading