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

Use unspecified QoS in QueueScheduler when QoS is unspecified #880

Merged
merged 1 commit into from
Mar 19, 2024

Conversation

jamieQ
Copy link
Contributor

@jamieQ jamieQ commented Oct 22, 2023

Issue

after doing a bit of research on how libdispatch's 'quality of service' (QoS) propagation logic works, i noticed that the default behavior of QueueScheduler is slightly different than what one would get if just using a DispatchQueue. i'm not sure if this was intentional, but in practice the consequence is that if you do something like this:

// assume we're on the main dispatch queue, i.e. QoS is 'user interactive'
let scheduler = QueueScheduler()
scheduler.schedule {
    // do something...
}

you end up with the inferred QoS of the scheduled block being set to 'default' QoS, whereas it would be 'user initiated' if using a vanilla DispatchQueue. this patch changes the default qos parameter from .default to .unspecified to match DispatchQueue's default behavior. this change might alter behavior of existing code; hopefully not in a detrimental way, but rather will apply the appropriate QoS propagation that one would assume would occur by default. unfortunately, i realize it's probably impossible to determine if this will cause undesirable changes in existing client code, so i'm open to a different approach or not making this change (though IMO it is semantically more appropriate).

Description

  • changes the default value of the QueueScheduler's qos parameter from .default to .unspecified
  • adds a unit test to validate QoS propagation works as expected with the change

Checklist

  • Updated CHANGELOG.md.

@jamieQ jamieQ marked this pull request as ready for review October 23, 2023 02:44
@Vyazovoy
Copy link

Vyazovoy commented Feb 4, 2024

This change looks pretty logical. If I don't specify the QoS then it should be .unspecified. Regarding the potential changes in existing codebases, I think that even if it happens it is for the better as it will discover the places that should have explicit QoS provided.

@Vyazovoy
Copy link

Vyazovoy commented Feb 4, 2024

@NachoSoto, could you please consider merging and releasing this change?

@jamieQ
Copy link
Contributor Author

jamieQ commented Feb 9, 2024

thanks @Vyazovoy & @NachoSoto. i see the required statuses haven't completed for some reason – is there anything else i need to do?

@mluisbrown
Copy link
Contributor

@jamieQ a maintainer has to approve running the checks (to avoid spamming the CI). I have in the past been able to do that, but either I have been removed as a maintainer or something has changed in GitHub about how you approve running the checks.

expect(initialQoS).toEventuallyNot(beNil())
expect(endQoS).toEventuallyNot(beNil())

expect(initialQoS).to(equal(QOS_CLASS_USER_INITIATED))
Copy link
Contributor

Choose a reason for hiding this comment

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

@jamieQ this is failing the Linux CI job. You probably need to to add this at the top of this file:

#if canImport(Darwin)
    import Darwin
#endif

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ah, sorry about that (and thank you for trying to progress this). scouring the internet for a bit made me wonder if the same QoS propagation mechanisms even exist on Linux. assuming they don't, perhaps just wrapping the runtime checks here in that conditional compilation directive would be an appropriate resolution. i'll try to do a bit more research in the near term.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@mluisbrown okay i think it should be good to re-run in CI at your convenience

@jamieQ jamieQ force-pushed the default-vs-unspecified-qos branch from 20a9cce to 46f953c Compare March 19, 2024 12:50
@mluisbrown mluisbrown merged commit f4f3d4d into ReactiveCocoa:master Mar 19, 2024
5 checks passed
@jamieQ jamieQ deleted the default-vs-unspecified-qos branch March 19, 2024 16:12
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.

4 participants