Full compatibility with -strict-concurrency=complete#125
Full compatibility with -strict-concurrency=complete#125
-strict-concurrency=complete#125Conversation
gwynne
left a comment
gwynne
left a comment
There was a problem hiding this comment.
See comments, also I'm not really happy with the SendableBox, too many locks... but I don't know that I have any better suggestions. This package is easily the worst mess left in Vapor now that Fluent and some other things have been cleaned up some...
|
|
||
| var __isEnabled = true | ||
| /// Global setting for enabling or disabling the cache | ||
| public var isEnabled: Bool = true | ||
| public var _isEnabled: Bool { | ||
| get { | ||
| self.lock.withLock { | ||
| self.__isEnabled | ||
| } | ||
| } | ||
| set(newValue) { | ||
| self.lock.withLock { | ||
| self.__isEnabled = newValue | ||
| } | ||
| } | ||
| } | ||
| /// Global setting for enabling or disabling the cache | ||
| public var isEnabled: Bool { | ||
| get { | ||
| self._isEnabled | ||
| } | ||
| set(newValue) { | ||
| self._isEnabled = newValue | ||
| } | ||
| } | ||
| /// Current count of cached documents |
There was a problem hiding this comment.
i assume compiler will take care of that anyway... two levels to not use self.lock.withLock 2 more times, which is also less code.
0xTim
left a comment
0xTim
left a comment
There was a problem hiding this comment.
We should add the flag as a step in CI too
Sources/LeafKit/SendableBox.swift
Outdated
| import NIOConcurrencyHelpers | ||
|
|
||
| /// Uses a locking mechanism to ensure Sendability. | ||
| internal final class SendableBox<Value>: @unchecked Sendable { |
There was a problem hiding this comment.
Why are we not using the NIO helpers? This doesn't make un-Sendable objects Sendable unfortunately as I've learn't the hard way
There was a problem hiding this comment.
What types are you thinking of? I don't think we can use types like NIOLoopBound since it requires an eventloop
There was a problem hiding this comment.
At least not everywhere. Like in global vars.
There was a problem hiding this comment.
NIOLockedValueBox or NIOLockedBox
There was a problem hiding this comment.
I forgot to add a Value: Sendable constraint. I assume that solves most of the problems?
There was a problem hiding this comment.
Use NIOLockedValueBox then rather than rolling our own. This will cause issues with userInfo as I'm sure you'll see
There was a problem hiding this comment.
ah ... the usual problem of outdated comments.
yeah i didn't remember that type exists.
Sources/LeafKit/LeafRenderer.swift
Outdated
| public let sources: LeafSources | ||
| /// The NIO `EventLoop` on which this instance of `LeafRenderer` will operate | ||
| public let eventLoop: EventLoop | ||
| let _userInfo: SendableBox<[AnyHashable: Any]> |
There was a problem hiding this comment.
This doesn't do what you think it does. The only way we can make Any Sendable is by tying this to an event loop
There was a problem hiding this comment.
Makes sense ... even with a Value: Sendable constraint it is still showing warnings.
Not sure what exactly to do though.
We need to change Any -> any Sendable as well as somehow conforming AnyHashable to Sendable.
There was a problem hiding this comment.
Though not sure what an event loop does that the locking mechanism can't ... I'm tempted to ask in the OpenSource slack.
There was a problem hiding this comment.
The documentation of NIOLoopBound does mention why...
|
@gwynne should the reusable CI have an option for strict concurrency checking? (automatically set to |
| public let userInfo: [AnyHashable: Any] | ||
|
|
||
| public var userInfo: [AnyHashable: Any] { | ||
| _userInfo.value |
There was a problem hiding this comment.
Will this crash on access by users on another eventLoop?
|
|
|
It appears for a dictionary like that, Foundation itself just uses |
|
Err @fabianfett that's wrong right? @MahdiBM the issue is if you store something that's not |
|
Yeah it's suboptimal, but it's something 😅 |
|
Nah it's really bad as Fabian explained to me 😅 essentially you go from the compiler giving you a warning saying this might be a problem to the user to the compiler completely removing that warning so the end user never knows. I'd rather have a dictionary that takes a Sendable type and pass the warning on to the user |
3 participants
These changes will make
leaf-kitfully compatible with-strict-concurrency=complete.