Skip to content

fix: polling 304 responses handled appropriately - #345

Merged
kinyoklion merged 1 commit into
mainfrom
ta/issue-344/polling-304-nullreference
Sep 3, 2026
Merged

fix: polling 304 responses handled appropriately#345
kinyoklion merged 1 commit into
mainfrom
ta/issue-344/polling-304-nullreference

Conversation

@tanderson-ld

@tanderson-ld tanderson-ld commented Sep 3, 2026

Copy link
Copy Markdown
Contributor

Requirements

  • I have added test coverage for new or changed functionality
  • I have followed the repository's pull request submission guidelines
  • I have validated my changes against all supported platform versions

Related issues

Fixes #344. Thanks to @cda210 for the report and for pinpointing the root cause — this PR is the one-line fix they proposed, plus a regression test.

Describe the solution you've provided

FeatureRequestor.GetAllDataAsync() signals "payload unchanged" by returning a null DataSetWithHeaders (its doc comment: "or null if they have not been modified"). PollingDataSource.UpdateTaskAsync() dereferenced dataAndHeaders.DataSet without first checking the wrapper reference, so every poll that received an HTTP 304 threw a NullReferenceException. It was swallowed by the generic catch (Exception), logged at Warning, and reported as DataSourceState.Interrupted.

The guard now checks the wrapper before the payload:

if (dataAndHeaders is null || dataAndHeaders.DataSet is null)

Worth noting the impact is slightly worse than "spurious warnings": because every unchanged poll takes this path, a polling client whose flags are stable sits permanently in Interrupted after its second poll, which matters for anyone gating a health check on DataSourceStatusProvider. Flag evaluation is unaffected, since the store already holds data from the prior successful poll.

Origin of the regression

Introduced in #81 (environment ID support for hooks), which wrapped the requestor's return value in DataSetWithHeaders so headers could ride along. That turned one nullable into two — the outer reference and the inner FullDataSet<ItemDescriptor>? — and the caller's check was rewritten onto the inner one while the requestor kept signalling 304 via the outer one:

https://github.com/launchdarkly/dotnet-core/pull/81/changes#diff-eff45aea6b7a68df4fc34a2d803960905666dd3995eea912053b1a7171129821R65

(Stable permalink to the same line, in case the diff anchor drifts: PollingDataSource.cs#L65 @ 49149f2.)

Before that change the check was simply if (allData is null), against a Task<FullDataSet<ItemDescriptor>?> return type. The bug has therefore been present since 8.7.0.

Describe alternatives you've considered

  • if (dataAndHeaders is null) alone — this is the strictly minimal inversion of feat: Add environment ID support for hooks. #81, since the inner DataSet is null arm is provably unreachable: new DataSetWithHeaders(...) is constructed in exactly one place and always with a non-null FullDataSet value, FeatureRequestor is the only implementation of IFeatureRequestor, and an empty response body throws JsonException rather than yielding a null DataSet. I kept the disjunction to match the fix proposed in the issue and because it stays correct if a second implementation is ever added. Happy to tighten it if reviewers prefer.
  • Fixing it in the requestor instead, by returning new DataSetWithHeaders(null, null) on a 304 so the existing check becomes correct. Rejected: a wrapper carrying a null payload is a weaker contract than a null wrapper, and it would contradict the requestor's documented return value.

Additional context

The existing 304 coverage (InitIsNotRepeatedIfServerReturnsNotModifiedStatus, ResponseWithNewEtagUpdatesEtag) could not catch this — those tests assert only that Init is not re-called and that ETags are echoed, and a thrown NullReferenceException produces "no Init" just as successful 304 handling does. The new test asserts the parts that were actually wrong: that every published status is Valid with no LastError, and that nothing is logged at Warning.

Verified: the new test fails on main with Interrupted / NullReferenceException and passes with the fix. Full server SDK unit suite green — 1606/1606 on net8.0.

The FDv2 polling data source is not affected (FDv2PollingDataSource handles response == null explicitly), nor is the client-side SDK (FeatureFlagRequestor returns a non-null WebResponse(304, null, ...)).


Note

Overview
Fixes a NullReferenceException when the polling data source receives unchanged flag data (HTTP 304): GetAllDataAsync() returns a null DataSetWithHeaders, but UpdateTaskAsync only checked DataSet and dereferenced the wrapper first.

The guard is now dataAndHeaders is null || dataAndHeaders.DataSet is null, so unchanged polls report DataSourceState.Valid instead of being caught as a generic failure and left in Interrupted with warning logs.

Adds StatusRemainsValidIfServerReturnsNotModifiedStatus to lock in valid status, no LastError, and no “polling failed” warnings across repeated 304s.

Reviewed by Cursor Bugbot for commit 6c932e1. Bugbot is set up for automated code reviews on this repo. Configure here.

FeatureRequestor.GetAllDataAsync() signals an unchanged payload by
returning a null DataSetWithHeaders, but PollingDataSource dereferenced
dataAndHeaders.DataSet without first checking the wrapper itself. Every
poll that received a 304 therefore threw a NullReferenceException, which
was swallowed by the generic handler, logged as a warning, and reported
as DataSourceState.Interrupted -- leaving a polling client with stable
flags permanently in an Interrupted state.

The inner DataSet null check is retained since it is harmless, but note
that it is currently unreachable: DataSetWithHeaders is only ever
constructed with a non-null data set.
@tanderson-ld
tanderson-ld requested a review from a team as a code owner September 3, 2026 19:14
@tanderson-ld tanderson-ld changed the title fix: Avoid NullReferenceException on polling 304 responses fix: polling 304 responses handled appropriately Sep 3, 2026
@kinyoklion
kinyoklion merged commit 74ad49c into main Sep 3, 2026
18 checks passed
@kinyoklion
kinyoklion deleted the ta/issue-344/polling-304-nullreference branch September 3, 2026 20:10
kinyoklion pushed a commit that referenced this pull request Sep 3, 2026
🤖 I have created a release *beep* *boop*
---


##
[8.16.2](LaunchDarkly.ServerSdk-v8.16.1...LaunchDarkly.ServerSdk-v8.16.2)
(2026-09-03)


### Bug Fixes

* polling 304 responses handled appropriately
([#345](#345))
([74ad49c](74ad49c))

---
This PR was generated with [Release
Please](https://github.com/googleapis/release-please). See
[documentation](https://github.com/googleapis/release-please#release-please).

<!-- CURSOR_SUMMARY -->
---

> [!NOTE]
> **Overview**
> Release Please PR that bumps **LaunchDarkly.ServerSdk** from `8.16.1`
to **`8.16.2`** in `.release-please-manifest.json`,
`LaunchDarkly.ServerSdk.csproj`, `PROVENANCE.md`, and `CHANGELOG.md`.
There are **no application source changes** in this diff—only release
metadata.
> 
> The new changelog entry documents the shipped fix: **polling treats
HTTP 304 (Not Modified) correctly**
([#345](#345)), so
conditional poll requests that return “not modified” no longer misbehave
as errors or failed updates.
> 
> <sup>Reviewed by [Cursor Bugbot](https://cursor.com/bugbot) for commit
79bb072. Bugbot is set up for automated
code reviews on this repo. Configure
[here](https://www.cursor.com/dashboard/bugbot).</sup>
<!-- /CURSOR_SUMMARY -->

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
evgenygunko pushed a commit to evgenygunko/Translations that referenced this pull request Sep 4, 2026
This PR contains the following updates:

| Package | Change | [Age](https://docs.renovatebot.com/merge-confidence/) | [Confidence](https://docs.renovatebot.com/merge-confidence/) |
|---|---|---|---|
| [LaunchDarkly.ServerSdk](https://github.com/launchdarkly/dotnet-core) | `8.16.1` → `8.16.2` | ![age](https://developer.mend.io/api/mc/badges/age/nuget/LaunchDarkly.ServerSdk/8.16.2?slim=true) | ![confidence](https://developer.mend.io/api/mc/badges/confidence/nuget/LaunchDarkly.ServerSdk/8.16.1/8.16.2?slim=true) |

---

### Release Notes

<details>
<summary>launchdarkly/dotnet-core (LaunchDarkly.ServerSdk)</summary>

### [`v8.16.2`](https://github.com/launchdarkly/dotnet-core/releases/tag/LaunchDarkly.ServerSdk-v8.16.2): LaunchDarkly.ServerSdk: v8.16.2

[Compare Source](launchdarkly/dotnet-core@LaunchDarkly.ServerSdk-v8.16.1...LaunchDarkly.ServerSdk-v8.16.2)

##### Bug Fixes

- polling 304 responses handled appropriately ([#&#8203;345](launchdarkly/dotnet-core#345)) ([74ad49c](launchdarkly/dotnet-core@74ad49c))

***

This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please).

<!-- CURSOR_SUMMARY -->

</details>

---

### Configuration

📅 **Schedule**: Branch creation - At any time (no schedule defined), Automerge - At any time (no schedule defined).

🚦 **Automerge**: Disabled by config. Please merge this manually once you are satisfied.

♻ **Rebasing**: Whenever PR becomes conflicted, or PR is renamed to start with "rebase!".

👻 **Immortal**: This PR will be recreated if closed unmerged. Get [config help](https://github.com/renovatebot/renovate/discussions) if that's undesired.

---

This PR has been generated by [Renovate Bot](https://github.com/renovatebot/renovate).
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.

Polling data source throws NullReferenceException on every HTTP 304 (Not Modified) response

3 participants