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

Potential deadlock when using ImmediateScheduler #1

Open
orj opened this issue Feb 9, 2022 · 1 comment
Open

Potential deadlock when using ImmediateScheduler #1

orj opened this issue Feb 9, 2022 · 1 comment

Comments

@orj
Copy link

orj commented Feb 9, 2022

I was running your package past our resident Combine expert and they mentioned:

It uses a single lock for downstream / upstream comms, so there are a number of scenarios that could result in deadlock. My guess it is doesn't support and hasn't been tested with the ImmediateScheduler. Thats generally a surefire way to find bugs with custom scheduling publishers.

I've not run any tests myself but thought I'd leave this feedback.

Hope it helps.

@elfenlaid
Copy link
Owner

elfenlaid commented Feb 9, 2022

Oh, that's for reaching out with the feedback 🙇

I've tested it with the ImmediateScheduler and the library seems to do fine. I wonder maybe it's something wrong with my test snippet?

import RateLimiter
import CombineSchedulers
import Combine

var cancellables = Set<AnyCancellable>()
defer { cancellables.removeAll() }

let scheduler = DispatchQueue.immediateScheduler
let strategy = QueueThroughputStrategy(rate: UInt(2), interval: .seconds(1), scheduler: scheduler)

var values: [Int] = []

(1...5).publisher
    .rateLimited(by: strategy)
    .sink(receiveValue: {
        values.append($0)
    })
    .store(in: &cancellables)

I just tweaked the snipped in the project's playground. You can clone the repo, open it with xed . and find the playground at Playgrounds > RateLimiter

CombineSchedulers is a library from https://github.com/pointfreeco/combine-schedulers which is really awesome, certainly worth having a look 👍

UPD:
You might also want to check my blog post about the library, more precisely about some assumptions it takes https://thegoodalright.dev/posts/combine-rate-limiting/#here-be-dragons

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

No branches or pull requests

2 participants