Skip to content

docs: Correct the socket_factory open() signature - #472

Draft
kinyoklion wants to merge 1 commit into
mainfrom
rlamb/sdk-3369/socket-factory-docs
Draft

docs: Correct the socket_factory open() signature#472
kinyoklion wants to merge 1 commit into
mainfrom
rlamb/sdk-3369/socket-factory-docs

Conversation

@kinyoklion

@kinyoklion kinyoklion commented Oct 23, 2026

Copy link
Copy Markdown
Member

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

SDK-3369

Describe the solution you've provided

Config#socket_factory was documented as requiring open(uri, timeout). The SDK passes the factory to http.rb as socket_class, and http.rb calls it as socket_class.open(host, port). That is true on every http.rb version the gemspec allows (>= 4.4.0, < 7.0.0), and has been since the option was added in 6.0.0.

A factory written to the documented signature gets a host name where it expects a URI, and a port where it expects a timeout. Depending on how it is written, it either:

  • raises NoMethodError on uri.host; or
  • takes its port from somewhere else and connects to the wrong place.

The second case is quiet: in a local check, events went to whatever server was on the factory's own port, and the SDK reported the send as successful.

Changes:

  • Config#socket_factory docs now:
    • give the real signature, open(host, port);
    • say those are the host and port to connect to, or the proxy's when a proxy is configured;
    • say the SDK performs TLS over the returned stream, so the stream must not already be encrypted;
    • say the SDK's connect timeout applies to the call. http.rb wraps the call in Timeout.timeout in 4.4.1, 5.3.1 and 6.0.4.
  • New note on FDv2 data sources: data sources created with the LaunchDarkly::DataSystem builders don't use Config#socket_factory. Each builder has its own socket_factory, and build_http_config only reads that.
  • DataSourceBuilderCommon#socket_factory docs: give the signature and point to Config#socket_factory.
  • Spec helper SocketFactoryFromHash: it declared open(uri, timeout) and ignored its second argument, mapping host names to ports. It now maps host names to addresses and connects to the port it is given. The two specs that use it put the server port in the URI, so they depend on the port argument.

Describe alternatives you've considered

Making the FDv2 builders fall back to Config#socket_factory. That's a behavior change, so I left it out of this docs fix.

Additional context

Testing:

  • spec/impl/event_sender_spec.rb and spec/ldclient_end_to_end_spec.rb pass on Ruby 3.2.3 with http.rb 6.0.4.
  • With the helper changed to ignore the port it's given:
    • the event sender spec fails;
    • the end-to-end spec never completes, because it waits for an events request with no timeout.
  • The full suite gave 1241 examples with 1 failure: can be automatically loaded by Bundler.require. That's local only: its inline Gemfile resolves against system gems, which here lack aws-sdk-dynamodb.
  • Rubocop is clean on the changed files, and YARD resolves the new links.

Follow-up: ld-eventsource has the same open(uri, timeout) wording (lib/ld-eventsource/client.rb:98).

@kinyoklion
docs: Correct the socket_factory open() signature
7394abe 
The socket_factory docs said the factory must provide open(uri, timeout).
The SDK hands the factory to http.rb as its socket class, and http.rb has
always called it as open(host, port): the host and port to connect to, or
the proxy's when a proxy is configured. A factory written to the documented
signature would get a host name where it expected a URI and a port where it
expected a timeout.

Document the real signature, that the SDK performs TLS over the returned
stream, and that the connect timeout applies to the call. Also note that
data sources built with the DataSystem builders use their own
socket_factory, not Config#socket_factory.

The spec helper SocketFactoryFromHash had the same misleading parameter
names and ignored the port it was given. It now maps host names to
addresses and connects to the port the HTTP client passes in, so the specs
that use it depend on that argument.
@kinyoklion kinyoklion mentioned this pull request Oct 23, 2026
Merged
2 of 3 tasks
kinyoklion added a commit that referenced this pull request Nov 2, 2026
@kinyoklion
test: Remove the poller race from the big segment status listener spec (
d3f643e 
#473)

Fixes an intermittent JRuby failure in
`big_segment_store_status_provider sends status updates`
(`spec/ldclient_listeners_spec.rb:34`, `expected false, got true`), seen
on #472.

- `spec/mock_components.rb`: adds `GatedBigSegmentStore`, a Big Segment
store whose metadata queries wait for the test to supply each result
- `spec/ldclient_listeners_spec.rb`: the example adds its observer and
checks what it received only while the poller is waiting inside a query

**Requirements**

- [x] I have added test coverage for new or changed functionality
- [x] I have followed the repository's [pull request submission
guidelines](../blob/main/CONTRIBUTING.md#submitting-pull-requests)
- [ ] I have validated my changes against all supported platform
versions

<details>
<summary>Implementation details</summary>

**Root cause**

The status poller caches each status inside `@poll_lock` and notifies
observers after it releases the lock. `status_provider.status` returns
the cached status without waiting for that notification. When the
poller's first poll runs before the example reads the status, as it
usually does on JRuby, the example can do all of the following before
the poller delivers its first notification:

1. Read the healthy status.
2. Add its observer.
3. Inject the store error.

The provider has not notified anyone yet, so that first notification
goes to the new observer, carrying the healthy status. `statuses.pop`
returns it instead of the unavailable status.

#440 fixed the same race in the `status polling` examples in
`spec/impl/big_segments_spec.rb`. There it caused a hang; here it
delivers the wrong first status.

**Solution**

`GatedBigSegmentStore#get_metadata` counts the query, then waits until
the test queues a result. The poller notifies observers of one result
before it starts its next query. So once query N has started, every
earlier result has been delivered, and nothing else happens until the
test queues another result.

The example now runs in three steps:

1. It queues a healthy result and reads it through
`status_provider.status`.
2. It waits for query 2, then adds the observer. Only the poller can
make that query, because later status reads come from the cache.
3. It queues an error and waits for query 3. Then it checks that the
observer received exactly one status, that the status is unavailable,
and that `status_provider.status` returns it.

None of these steps depends on how long a poll or notification takes:
- `status_poll_interval: 0.01` only controls how quickly the poller
moves on to its next query.
- The 5-second wait timeouts only bound how long a failure takes to
report.

The example releases the store in `ensure`. The client can't stop its
poller while a query is waiting.

**Alternatives considered**

- Skipping statuses until an unavailable one arrives, as
`next_status_matching` does in `big_segments_spec.rb`. That would pass,
but it would hide an extra or out-of-order notification, and it waits in
a sleep loop.
- Changing the manager so the first status reaches observers before
`status` returns. That changes SDK behavior to suit a test. The manager
also deliberately notifies outside `@poll_lock` (see
`query_store_status`).

**Testing**

With no code changes, the failure reproduces on JRuby 9.4.6 pinned to
one or two cores. Every failure was the CI one: `expected false, got
true` at line 34.

| Unmodified SDK, JRuby 9.4.6 | Old example | New example |
|---|---|---|
| Pinned to 1 core (`taskset -c 0`) | 6/1,000 failed; 8/1,000 on a rerun
| 1,000/1,000 passed |
| Pinned to 2 cores | 4/1,000 failed | 1,000/1,000 passed |
| 8 cores, not pinned | 1,500/1,500 passed | 1,500/1,500 passed |

The next rows inject delays, gated by environment variables, into a
local copy of `big_segments.rb` and the spec. None of that code is in
this PR. The random delays apply at 5 points in the poll and notify
path, plus the example's status read and `add_observer`.

| Injected delays | Old example | New example |
|---|---|---|
| Poller polls first, notify delayed 0.3s (MRI 3.2) | 10/10 failed |
10/10 passed |
| Same, JRuby 9.4.6 | 10/10 failed | 10/10 passed |
| Random delay up to 5ms at each point (MRI 3.2) | 121/300 failed |
300/300 passed |
| Same, JRuby 9.4.6 | 141/300 failed | 300/300 passed |
| Random delay up to 50ms at each point (MRI 3.2) | 23/60 failed | 60/60
passed |
| Same, JRuby 9.4.6 | 23/60 failed | 60/60 passed |

The new example still fails when the SDK is broken in each of these
ways:
- The poller never notifies.
- A store error is reported as available.
- A change is notified twice.
- Polls never update the cached status.
- The store is queried only once.

The full suite passes on MRI 3.2 (`bundle exec rspec spec`) and on JRuby
9.4.6 (`--tag '~flaky'`, as in CI), with one exception.
`launchdarkly-server-sdk_spec.rb:5` fails on `main` too: its unbundled
subprocess can't find the gems in my local bundle path. RuboCop reports
no offenses on the changed files.

**Not addressed here**

The same timing can affect an application. If it reads the status and
then adds a listener while the client is starting, the listener can
receive the status that was just read. That's a redundant notification
rather than a wrong status, so this PR doesn't change it.

</details>

[SDK-3424](https://launchdarkly.atlassian.net/browse/SDK-3424)


[SDK-3424]:
https://launchdarkly.atlassian.net/browse/SDK-3424?atlOrigin=eyJpIjoiNWRkNTljNzYxNjVmNDY3MDlhMDU5Y2ZhYzA5YTRkZjUiLCJwIjoiZ2l0aHViLWNvbS1KU1cifQ

<!-- CURSOR_SUMMARY -->
---

> [!NOTE]
> **Overview**
> Fixes a flaky JRuby failure in **`big_segment_store_status_provider
sends status updates`** where the observer could receive the initial
healthy status instead of the post-error unavailable status, because the
status poller notifies observers after caching and the old mock returned
results immediately.
> 
> Adds **`GatedBigSegmentStore`** in `spec/mock_components.rb`: metadata
**`get_metadata`** blocks until the test **`queue_metadata`** /
**`queue_metadata_error`**, exposes **`wait_for_queries`** for poll
ordering, and **`release`** so the client can shut down without hanging
on a blocked query.
> 
> The listener example switches from **`MockBigSegmentStore`** to the
gated store: queue healthy metadata, wait for the second poll before
registering the observer, then queue an error and wait for the third
poll before asserting exactly one unavailable notification and matching
**`status`**.
> 
> <sup>Reviewed by [Cursor Bugbot](https://cursor.com/bugbot) for commit
1ecf5d0. Bugbot is set up for automated
code reviews on this repo. Configure
[here](https://www.cursor.com/dashboard/bugbot).</sup>
<!-- /CURSOR_SUMMARY -->

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

1 participant

@kinyoklion