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.

Sign up for GitHub

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

Jump to bottom

Add jitter support to recurring tasks #2542

Merged
merged 9 commits into from
Oct 13, 2023
Merged

Add jitter support to recurring tasks #2542

merged 9 commits into from
Oct 13, 2023

Conversation

LorenzoFritzsch
Copy link
Contributor

@LorenzoFritzsch LorenzoFritzsch commented Oct 9, 2023 •
edited
Loading

Add jitter support to functions managing recurring tasks.

Motivation:

Closes #2505.

Modifications:

Overloaded the following EventLoop functions that might need jitter support:

  • flatScheduleTask
  • scheduleRepeatedTask
  • scheduleRepeatedAsyncTask

with the following new parameter: maximumAllowableJitter: TimeAmount, which is the exclusive upper bound of the jitter range that will be added to the delay and initialDelay parameters.

Result:

If jitter is needed when scheduling a repeated task with one of the functions listed above, it will be possible to add it by using the maximumAllowableJitter parameter.

@LorenzoFritzsch LorenzoFritzsch changed the title Add jitter support - Issue #2505 Add jitter support Oct 9, 2023
Lukasa
Lukasa requested changes Oct 10, 2023
View reviewed changes
Copy link
Contributor

@Lukasa Lukasa left a comment

Choose a reason for hiding this comment

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

Thanks, this code looks broadly correct! Can we add some unit tests to validate their correct behaviour? EmbeddedEventLoop will be useful for this as it has a fake clock.

Sources/NIOCore/EventLoop.swift Outdated
@discardableResult
@inlinable
@preconcurrency
public func flatScheduleTask<T>(
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we also add an extension that offers a variation of scheduleTask with jitter?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

A function that schedules a task after a given amount of time, with that amount being jittered? If this is the case I don't see the difference with this overloaded function, and as you pointed out as there's no rescheduling jittering should be unnecessary.

Sources/NIOCore/EventLoop.swift Outdated
/// Schedule a `task` that is executed by this `EventLoop` after the given amount of time.
///
/// - parameters:
/// - delay: The delay between the end of one task and the start of the next.
Copy link
Contributor

Choose a reason for hiding this comment

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

This seems wrong, as flatScheduleTask doesn't reschedule anything.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I misunderstood the nature of this function, I will remove this overload as it's unnecessary.

Copy link
Contributor

Choose a reason for hiding this comment

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

Sure, if we remove this overload then that's fine.

Sources/NIOCore/EventLoop.swift Outdated
/// - note: You can only cancel a task before it has started executing.
@discardableResult
@inlinable
@preconcurrency
Copy link
Contributor

Choose a reason for hiding this comment

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

This is not @preconcurrency.

Sources/NIOCore/EventLoop.swift Outdated
_ task: @escaping @Sendable (RepeatedTask) throws -> Void
) -> RepeatedTask {
let jitteredDelay = self._getJitteredDelay(delay: delay, maximumAllowableJitter: maximumAllowableJitter)
return self.scheduleRepeatedTask(initialDelay: initialDelay, delay: jitteredDelay, notifying: promise, task)
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we jitter the initial delay as well?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes will do, jittering the initial delay will avoid a first load spike, which is part of what this PR should fix

Sources/NIOCore/EventLoop.swift Outdated Show resolved Hide resolved
Lukasa
Lukasa reviewed Oct 12, 2023
View reviewed changes
Copy link
Contributor

@Lukasa Lukasa left a comment

Choose a reason for hiding this comment

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

Looking great, just a quick note.

Sources/NIOCore/EventLoop.swift Outdated
/// - task: The closure that will be executed.
/// - return: `RepeatedTask`
@discardableResult
@preconcurrency
Copy link
Contributor

Choose a reason for hiding this comment

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

We can remove the @preconcurrency annotations here, these functions are new.

Sources/NIOCore/EventLoop.swift Outdated
///
/// - return: `RepeatedTask`
@discardableResult
@preconcurrency
Copy link
Contributor

Choose a reason for hiding this comment

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

Same preconcurrency note here.

@LorenzoFritzsch LorenzoFritzsch changed the title Add jitter support Add jitter support to recurring tasks Oct 12, 2023
Lukasa
Lukasa approved these changes Oct 12, 2023
View reviewed changes
Copy link
Contributor

@Lukasa Lukasa left a comment

Choose a reason for hiding this comment

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

Nice one, LGTM.

@Lukasa Lukasa added the semver/minor Adds new public API. label Oct 12, 2023
@Lukasa
Copy link
Contributor

Lukasa commented Oct 12, 2023

@swift-server-bot test this please

@Lukasa
Copy link
Contributor

Lukasa commented Oct 12, 2023

Couple of build errors need to be cleaned up I'm afraid:

18:28:18 /swift-nio/Tests/NIOPosixTests/EventLoopTest.swift:394:13: error: mutation of captured var 'sentinel' in concurrently-executing code
18:28:18             sentinel += 1
18:28:18             ^
18:28:18 /swift-nio/Tests/NIOPosixTests/EventLoopTest.swift:846:13: error: mutation of captured var 'sentinel' in concurrently-executing code
18:28:18             sentinel += 1
18:28:18             ^

@LorenzoFritzsch
Copy link
Contributor Author

@swift-server-bot test this please

1 similar comment
@Lukasa
Copy link
Contributor

Lukasa commented Oct 13, 2023

@swift-server-bot test this please

@Lukasa Lukasa merged commit 7954dba into apple:main Oct 13, 2023
8 checks passed
@LorenzoFritzsch LorenzoFritzsch deleted the 2505-jitter-support branch October 14, 2023 10:40
LorenzoFritzsch added a commit to LorenzoFritzsch/swift-nio that referenced this pull request Oct 14, 2023
@LorenzoFritzsch
Merge pull request #2 from apple/main
1ef3c42 
Add jitter support to recurring tasks (apple#2542)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Lukasa Lukasa Lukasa approved these changes

Assignees
No one assigned
Labels
semver/minor Adds new public API.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

schedule(Repeated)(Async)Task (and friends): jitter support
2 participants
@LorenzoFritzsch @Lukasa