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 start() re-entrancy (#1) #56

Merged
merged 1 commit into from
May 30, 2023
Merged

Fix start() re-entrancy (#1) #56

merged 1 commit into from
May 30, 2023

Conversation

g-mark
Copy link
Contributor

@g-mark g-mark commented May 19, 2023

Fixes an EventSource.start() re-entrancy problem

When:

  1. EventSource.start() is called
  2. EventSource.start() is called again, before the network response from the first call has been received

Then:

  • a second URLSession and URLRequest are created, and a new URLSessionDataTask is started

The underlying issue is that start() is checking for readyState == .raw, but readyState isn't set until a network response has been received.

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

None

Describe the solution you've provided

Set readyState = .connecting in start(), immediately after checking for readyState == .raw.

Describe alternatives you've considered

None

Additional context

None

* Fix start() re-entrancy
* Add re-entrancy unit test
@tanderson-ld
Copy link
Contributor

Hi there @g-mark , thank you for the contribution! I have it on my list to take a look at the changes and related code/behavior and if it looks good, we'll integrate the change.

@keelerm84 keelerm84 merged commit b016174 into launchdarkly:main May 30, 2023
keelerm84 added a commit that referenced this pull request Jun 5, 2023
## [3.1.0] - 2023-06-05
### Changed:
- Enforce TLS v1.2 as a required minimum.

### Fixed:
- Fix re-entrancy issue with `start` command. (Thanks, [g-mark](#56)!)
keelerm84 added a commit that referenced this pull request Jun 5, 2023
## [3.1.0] - 2023-06-05
### Changed:
- Enforce TLS v1.2 as a required minimum.

### Fixed:
- Fix re-entrancy issue with `start` command. (Thanks,
[g-mark](#56)!)
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.

3 participants