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

Concurrency update #99

Draft
wants to merge 14 commits into
base: main
Choose a base branch
from
Draft

Conversation

akbashev
Copy link

@akbashev akbashev commented Sep 5, 2024

Motivation

Library being written even before async/await. So now, even though it supports it partly, with upcoming Swift 6 release and strict concurrency we should update it.

Plan

First steps after looking at the state of library, should be straightforward (in a way):

  • Check what type should be Sendable. Things like messages, peers and nodes are first to look at.
  • Wonder if Codable implementation should be part of library itself, rather than each implementation doing on it's own.. I would even argue that SWIMPeer should be Codable, as it's being used in messages.
  • Remove DispatchTime in favour of Duration and ContinuationClock.Instant
  • Enable strict concurrency mode and check what else should be updated. Probably closures and classes, using Mutex and etc.
  • Update tests.
  • Update examples.
  • Update CI when final Swift 6 images will be released?!

Will assume, that when finished library will be compatible with Swift 6. But here comes a bit different question—could code and design be even more clear having async/await, actors and etc. To answer this question, as a next steps I want to remove EventLoop and Promises in SWIMNIOExample and see how it works. Based on that experience would be interesting to check if any design updates even needed.

Comment on lines 120 to 128
public enum GossipPayload<Peer: SWIMPeer>: Sendable {
public struct GossipPayload<Peer: SWIMPeer>: Codable, Sendable {
/// Explicit case to signal "no gossip payload"
///
/// Effectively equivalent to an empty `.membership([])` case.
case none
/// Gossip information about a few select members.
case membership([SWIM.Member<Peer>])
}
}
Copy link
Author

@akbashev akbashev Sep 5, 2024

Choose a reason for hiding this comment

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

As .none basically an Optional, I've decided to change GossipPayload from enum to struct having only members. All other parts of code should use Optional<GossipPayload<Peer>> or basically GossipPayload<Peer>>? as a more visible and Swifty way.

@akbashev
Copy link
Author

akbashev commented Sep 5, 2024

@ktoso I will create a draft PR, so it will be easier for me to return to this topic from time to time and not to forget. Basically it's already working at that stage, but I've been updating all the errors and warning reactively, so not much thought in it. :)
Now a time to think a bit more about design overall. Though, if you already have some thoughts on current progress—please feel free to comment.

@ktoso
Copy link
Member

ktoso commented Nov 7, 2024

We now have CI again so let's give this a review tomorrow and merge maybe :)

@ktoso
Copy link
Member

ktoso commented Nov 8, 2024

If you'd run

swift format . --recursive --in-place

that will resolve many of the conflicts i think

@akbashev
Copy link
Author

akbashev commented Nov 8, 2024

lot's of conflicts, will probably have a good time resolving

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.

2 participants