ReactiveSwift icon indicating copy to clipboard operation
ReactiveSwift copied to clipboard

Use unspecified QoS in QueueScheduler when QoS is unspecified

Open jamieQ opened this issue 2 years ago • 4 comments

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

  • [x] Updated CHANGELOG.md.

jamieQ avatar Oct 22 '23 18:10 jamieQ

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 avatar Feb 04 '24 02:02 Vyazovoy

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

Vyazovoy avatar Feb 04 '24 02:02 Vyazovoy

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

jamieQ avatar Feb 09 '24 21:02 jamieQ

@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.

mluisbrown avatar Feb 10 '24 10:02 mluisbrown