dnrops
/
swift-nio
1
0
Fork 0
Code Issues Pull Requests Packages Projects Releases Wiki Activity

remove/deprecate the file:line: parameters from flatMap and friends (#1998) 

Motivation:

For NIO's 'promise leak detector' we added file:line: labels to
makePromise and there it makes sense. A user might create a promise and
then never fulfil it, bad. With the file:line: arguments we can give
good diagnostics.

However, we (probably that @weissi again) also added it to flatMap and
friends where it doesn't make sense at all.

Sure, EventLoopFuture's implementation may create a promise in the
implementation of flatMap but this promise is never leaked unless the
previous future is never fulfilled (or NIO has a terrible bug). Suffice
to say that in a future chain, it's never a flatMap etc which is
responsible for leaking the first promise...

Explain here the context, and why you're making that change.
What is the problem you're trying to solve.

Modifications:

Remove all unnecessary `file:line:` parameters whilst keeping the public
API intact.

Result:

More sensible code.
This commit is contained in:
Johannes Weiss 2021-12-03 16:27:55 +00:00 committed by GitHub
parent 773b3797d7
commit 0697d5a599
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
9 changed files with 162 additions and 48 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

27
Sources/NIOCore/EventLoop+Deprecated.swift Normal file
View File

@ -0,0 +1,27 @@
//===----------------------------------------------------------------------===//
//
// This source file is part of the SwiftNIO open source project
//
// Copyright (c) 2021 Apple Inc. and the SwiftNIO project authors
// Licensed under Apache License v2.0
//
// See LICENSE.txt for license information
// See CONTRIBUTORS.txt for the list of SwiftNIO project authors
//
// SPDX-License-Identifier: Apache-2.0
//
//===----------------------------------------------------------------------===//
extension EventLoop {
@inlinable
@available(*, deprecated, message: "Please don't pass file:line:, there's no point.")
public func makeFailedFuture<T>(_ error: Error, file: StaticString = #file, line: UInt = #line) -> EventLoopFuture<T> {
return self.makeFailedFuture(error)
}
@inlinable
@available(*, deprecated, message: "Please don't pass file:line:, there's no point.")
public func makeSucceededFuture<Success>(_ value: Success, file: StaticString = #file, line: UInt = #line) -> EventLoopFuture<Success> {
return self.makeSucceededFuture(value)
}
}

10
Sources/NIOCore/EventLoop.swift
View File

@ -312,7 +312,7 @@ public protocol EventLoop: EventLoopGroup {
extension EventLoop {
/// Default implementation of `makeSucceededVoidFuture`: Return a fresh future (which will allocate).
public func makeSucceededVoidFuture() -> EventLoopFuture<Void> {
return EventLoopFuture(eventLoop: self, value: (), file: "n/a", line: 0)
return EventLoopFuture(eventLoop: self, value: ())
}
public func _preconditionSafeToWait(file: StaticString, line: UInt) {
@ -637,8 +637,8 @@ extension EventLoop {
/// - error: the `Error` that is used by the `EventLoopFuture`.
/// - returns: a failed `EventLoopFuture`.
@inlinable
public func makeFailedFuture<T>(_ error: Error, file: StaticString = #file, line: UInt = #line) -> EventLoopFuture<T> {
return EventLoopFuture<T>(eventLoop: self, error: error, file: file, line: line)
public func makeFailedFuture<T>(_ error: Error) -> EventLoopFuture<T> {
return EventLoopFuture<T>(eventLoop: self, error: error)
}
/// Creates and returns a new `EventLoopFuture` that is already marked as success. Notifications will be done using this `EventLoop` as execution `NIOThread`.
@ -647,12 +647,12 @@ extension EventLoop {
/// - result: the value that is used by the `EventLoopFuture`.
/// - returns: a succeeded `EventLoopFuture`.
@inlinable
public func makeSucceededFuture<Success>(_ value: Success, file: StaticString = #file, line: UInt = #line) -> EventLoopFuture<Success> {
public func makeSucceededFuture<Success>(_ value: Success) -> EventLoopFuture<Success> {
if Success.self == Void.self {
// The as! will always succeed because we previously checked that Success.self == Void.self.
return self.makeSucceededVoidFuture() as! EventLoopFuture<Success>
} else {
return EventLoopFuture<Success>(eventLoop: self, value: value, file: file, line: line)
return EventLoopFuture<Success>(eventLoop: self, value: value)
}
}

77
Sources/NIOCore/EventLoopFuture+Deprecated.swift Normal file
View File

@ -0,0 +1,77 @@
//===----------------------------------------------------------------------===//
//
// This source file is part of the SwiftNIO open source project
//
// Copyright (c) 2021 Apple Inc. and the SwiftNIO project authors
// Licensed under Apache License v2.0
//
// See LICENSE.txt for license information
// See CONTRIBUTORS.txt for the list of SwiftNIO project authors
//
// SPDX-License-Identifier: Apache-2.0
//
//===----------------------------------------------------------------------===//
extension EventLoopFuture {
@inlinable
@available(*, deprecated, message: "Please don't pass file:line:, there's no point.")
public func flatMap<NewValue>(file: StaticString = #file, line: UInt = #line, _ callback: @escaping (Value) -> EventLoopFuture<NewValue>) -> EventLoopFuture<NewValue> {
return self.flatMap(callback)
}
@inlinable
@available(*, deprecated, message: "Please don't pass file:line:, there's no point.")
public func flatMapThrowing<NewValue>(file: StaticString = #file,
line: UInt = #line,
_ callback: @escaping (Value) throws -> NewValue) -> EventLoopFuture<NewValue> {
return self.flatMapThrowing(callback)
}
@inlinable
@available(*, deprecated, message: "Please don't pass file:line:, there's no point.")
public func flatMapErrorThrowing(file: StaticString = #file, line: UInt = #line, _ callback: @escaping (Error) throws -> Value) -> EventLoopFuture<Value> {
return self.flatMapErrorThrowing(callback)
}
@inlinable
@available(*, deprecated, message: "Please don't pass file:line:, there's no point.")
public func map<NewValue>(file: StaticString = #file, line: UInt = #line, _ callback: @escaping (Value) -> (NewValue)) -> EventLoopFuture<NewValue> {
return self.map(callback)
}
@inlinable
@available(*, deprecated, message: "Please don't pass file:line:, there's no point.")
public func flatMapError(file: StaticString = #file, line: UInt = #line, _ callback: @escaping (Error) -> EventLoopFuture<Value>) -> EventLoopFuture<Value> {
return self.flatMapError(callback)
}
@inlinable
@available(*, deprecated, message: "Please don't pass file:line:, there's no point.")
public func flatMapResult<NewValue, SomeError: Error>(file: StaticString = #file,
line: UInt = #line,
_ body: @escaping (Value) -> Result<NewValue, SomeError>) -> EventLoopFuture<NewValue> {
return self.flatMapResult(body)
}
@inlinable
@available(*, deprecated, message: "Please don't pass file:line:, there's no point.")
public func recover(file: StaticString = #file, line: UInt = #line, _ callback: @escaping (Error) -> Value) -> EventLoopFuture<Value> {
return self.recover(callback)
}
@inlinable
@available(*, deprecated, message: "Please don't pass file:line:, there's no point.")
public func and<OtherValue>(_ other: EventLoopFuture<OtherValue>,
file: StaticString = #file,
line: UInt = #line) -> EventLoopFuture<(Value, OtherValue)> {
return self.and(other)
}
@inlinable
@available(*, deprecated, message: "Please don't pass file:line:, there's no point.")
public func and<OtherValue>(value: OtherValue,
file: StaticString = #file,
line: UInt = #line) -> EventLoopFuture<(Value, OtherValue)> {
return self.and(value: value)
}
}

72
Sources/NIOCore/EventLoopFuture.swift
View File

@ -156,6 +156,13 @@ public struct EventLoopPromise<Value> {
/// `EventLoopPromise` is completed.
public let futureResult: EventLoopFuture<Value>
@inlinable
internal static func makeUnleakablePromise(eventLoop: EventLoop, line: UInt = #line) -> EventLoopPromise<Value> {
return EventLoopPromise<Value>(eventLoop: eventLoop,
file: "BUG in SwiftNIO (please report), unleakable promise leaked.",
line: line)
}
/// General initializer
///
/// - parameters:
@ -386,9 +393,9 @@ public final class EventLoopFuture<Value> {
internal var _callbacks: CallbackList
@inlinable
internal init(_eventLoop eventLoop: EventLoop, value: Result<Value, Error>?, file: StaticString, line: UInt) {
internal init(_eventLoop eventLoop: EventLoop, file: StaticString, line: UInt) {
self.eventLoop = eventLoop
self._value = value
self._value = nil
self._callbacks = .init()
debugOnly {
@ -396,21 +403,20 @@ public final class EventLoopFuture<Value> {
}
}
@inlinable
internal convenience init(_eventLoop eventLoop: EventLoop, file: StaticString, line: UInt) {
self.init(_eventLoop: eventLoop, value: nil, file: file, line: line)
}
/// A EventLoopFuture<Value> that has already succeeded
@inlinable
internal convenience init(eventLoop: EventLoop, value: Value, file: StaticString, line: UInt) {
self.init(_eventLoop: eventLoop, value: .success(value), file: file, line: line)
internal init(eventLoop: EventLoop, value: Value) {
self.eventLoop = eventLoop
self._value = .success(value)
self._callbacks = .init()
}
/// A EventLoopFuture<Value> that has already failed
@inlinable
internal convenience init(eventLoop: EventLoop, error: Error, file: StaticString, line: UInt) {
self.init(_eventLoop: eventLoop, value: .failure(error), file: file, line: line)
internal init(eventLoop: EventLoop, error: Error) {
self.eventLoop = eventLoop
self._value = .failure(error)
self._callbacks = .init()
}
deinit {
@ -464,8 +470,8 @@ extension EventLoopFuture {
/// a new `EventLoopFuture`.
/// - returns: A future that will receive the eventual value.
@inlinable
public func flatMap<NewValue>(file: StaticString = #file, line: UInt = #line, _ callback: @escaping (Value) -> EventLoopFuture<NewValue>) -> EventLoopFuture<NewValue> {
let next = EventLoopPromise<NewValue>(eventLoop: eventLoop, file: file, line: line)
public func flatMap<NewValue>(_ callback: @escaping (Value) -> EventLoopFuture<NewValue>) -> EventLoopFuture<NewValue> {
let next = EventLoopPromise<NewValue>.makeUnleakablePromise(eventLoop: self.eventLoop)
self._whenComplete {
switch self._value! {
case .success(let t):
@ -500,10 +506,8 @@ extension EventLoopFuture {
/// a new value lifted into a new `EventLoopFuture`.
/// - returns: A future that will receive the eventual value.
@inlinable
public func flatMapThrowing<NewValue>(file: StaticString = #file,
line: UInt = #line,
_ callback: @escaping (Value) throws -> NewValue) -> EventLoopFuture<NewValue> {
let next = EventLoopPromise<NewValue>(eventLoop: eventLoop, file: file, line: line)
public func flatMapThrowing<NewValue>(_ callback: @escaping (Value) throws -> NewValue) -> EventLoopFuture<NewValue> {
let next = EventLoopPromise<NewValue>.makeUnleakablePromise(eventLoop: self.eventLoop)
self._whenComplete {
switch self._value! {
case .success(let t):
@ -535,8 +539,8 @@ extension EventLoopFuture {
/// a new value lifted into a new `EventLoopFuture`.
/// - returns: A future that will receive the eventual value or a rethrown error.
@inlinable
public func flatMapErrorThrowing(file: StaticString = #file, line: UInt = #line, _ callback: @escaping (Error) throws -> Value) -> EventLoopFuture<Value> {
let next = EventLoopPromise<Value>(eventLoop: eventLoop, file: file, line: line)
public func flatMapErrorThrowing(_ callback: @escaping (Error) throws -> Value) -> EventLoopFuture<Value> {
let next = EventLoopPromise<Value>.makeUnleakablePromise(eventLoop: self.eventLoop)
self._whenComplete {
switch self._value! {
case .success(let t):
@ -580,12 +584,12 @@ extension EventLoopFuture {
/// a new value lifted into a new `EventLoopFuture`.
/// - returns: A future that will receive the eventual value.
@inlinable
public func map<NewValue>(file: StaticString = #file, line: UInt = #line, _ callback: @escaping (Value) -> (NewValue)) -> EventLoopFuture<NewValue> {
public func map<NewValue>(_ callback: @escaping (Value) -> (NewValue)) -> EventLoopFuture<NewValue> {
if NewValue.self == Value.self && NewValue.self == Void.self {
self.whenSuccess(callback as! (Value) -> Void)
return self as! EventLoopFuture<NewValue>
} else {
let next = EventLoopPromise<NewValue>(eventLoop: eventLoop, file: file, line: line)
let next = EventLoopPromise<NewValue>.makeUnleakablePromise(eventLoop: self.eventLoop)
self._whenComplete {
return next._setValue(value: self._value!.map(callback))
}
@ -605,8 +609,8 @@ extension EventLoopFuture {
/// a new value lifted into a new `EventLoopFuture`.
/// - returns: A future that will receive the recovered value.
@inlinable
public func flatMapError(file: StaticString = #file, line: UInt = #line, _ callback: @escaping (Error) -> EventLoopFuture<Value>) -> EventLoopFuture<Value> {
let next = EventLoopPromise<Value>(eventLoop: eventLoop, file: file, line: line)
public func flatMapError(_ callback: @escaping (Error) -> EventLoopFuture<Value>) -> EventLoopFuture<Value> {
let next = EventLoopPromise<Value>.makeUnleakablePromise(eventLoop: self.eventLoop)
self._whenComplete {
switch self._value! {
case .success(let t):
@ -640,10 +644,8 @@ extension EventLoopFuture {
/// a new value or error lifted into a new `EventLoopFuture`.
/// - returns: A future that will receive the eventual value.
@inlinable
public func flatMapResult<NewValue, SomeError: Error>(file: StaticString = #file,
line: UInt = #line,
_ body: @escaping (Value) -> Result<NewValue, SomeError>) -> EventLoopFuture<NewValue> {
let next = EventLoopPromise<NewValue>(eventLoop: eventLoop, file: file, line: line)
public func flatMapResult<NewValue, SomeError: Error>(_ body: @escaping (Value) -> Result<NewValue, SomeError>) -> EventLoopFuture<NewValue> {
let next = EventLoopPromise<NewValue>.makeUnleakablePromise(eventLoop: self.eventLoop)
self._whenComplete {
switch self._value! {
case .success(let value):
@ -673,8 +675,8 @@ extension EventLoopFuture {
/// a new value lifted into a new `EventLoopFuture`.
/// - returns: A future that will receive the recovered value.
@inlinable
public func recover(file: StaticString = #file, line: UInt = #line, _ callback: @escaping (Error) -> Value) -> EventLoopFuture<Value> {
let next = EventLoopPromise<Value>(eventLoop: eventLoop, file: file, line: line)
public func recover(_ callback: @escaping (Error) -> Value) -> EventLoopFuture<Value> {
let next = EventLoopPromise<Value>.makeUnleakablePromise(eventLoop: self.eventLoop)
self._whenComplete {
switch self._value! {
case .success(let t):
@ -786,10 +788,8 @@ extension EventLoopFuture {
/// of results. If either one fails, the combined `EventLoopFuture` will fail with
/// the first error encountered.
@inlinable
public func and<OtherValue>(_ other: EventLoopFuture<OtherValue>,
file: StaticString = #file,
line: UInt = #line) -> EventLoopFuture<(Value, OtherValue)> {
let promise = EventLoopPromise<(Value, OtherValue)>(eventLoop: eventLoop, file: file, line: line)
public func and<OtherValue>(_ other: EventLoopFuture<OtherValue>) -> EventLoopFuture<(Value, OtherValue)> {
let promise = EventLoopPromise<(Value, OtherValue)>.makeUnleakablePromise(eventLoop: self.eventLoop)
var tvalue: Value?
var uvalue: OtherValue?
@ -830,10 +830,8 @@ extension EventLoopFuture {
/// Return a new EventLoopFuture that contains this "and" another value.
/// This is just syntactic sugar for `future.and(loop.makeSucceedFuture(value))`.
@inlinable
public func and<OtherValue>(value: OtherValue,
file: StaticString = #file,
line: UInt = #line) -> EventLoopFuture<(Value, OtherValue)> {
return and(EventLoopFuture<OtherValue>(eventLoop: self.eventLoop, value: value, file: file, line: line))
public func and<OtherValue>(value: OtherValue) -> EventLoopFuture<(Value, OtherValue)> {
return self.and(EventLoopFuture<OtherValue>(eventLoop: self.eventLoop, value: value))
}
}

13
Sources/NIOCrashTester/CrashTests+EventLoop.swift
View File

@ -91,4 +91,17 @@ struct EventLoopCrashTests {
f()
}
}
let testLeakingAPromiseCrashes = CrashTest(
regex: #"Fatal error: leaking promise created at"#
) {
@inline(never)
func leaker() {
_ = group.next().makePromise(of: Void.self)
}
leaker()
for el in group.makeIterator() {
try! el.submit {}.wait()
}
}
}

3
Sources/NIOEmbedded/Embedded.swift
View File

@ -206,8 +206,7 @@ public final class EmbeddedEventLoop: EventLoop {
public func _promiseCompleted(futureIdentifier: _NIOEventLoopFutureIdentifier) -> (file: StaticString, line: UInt)? {
precondition(_isDebugAssertConfiguration())
// The force-unwrap is safe: we know that we must have tracked all the futures.
return self._promiseCreationStore.removeValue(forKey: futureIdentifier)!
return self._promiseCreationStore.removeValue(forKey: futureIdentifier)
}
public func _preconditionSafeToSyncShutdown(file: StaticString, line: UInt) {

2
Sources/NIOPosix/SelectableEventLoop.swift
View File

@ -122,7 +122,7 @@ internal final class SelectableEventLoop: EventLoop {
internal func _promiseCompleted(futureIdentifier: _NIOEventLoopFutureIdentifier) -> (file: StaticString, line: UInt)? {
precondition(_isDebugAssertConfiguration())
return self.promiseCreationStoreLock.withLock {
self._promiseCreationStore.removeValue(forKey: futureIdentifier)!
self._promiseCreationStore.removeValue(forKey: futureIdentifier)
}
}

4
Tests/NIOPosixTests/EventLoopFutureTest.swift
View File

@ -25,13 +25,13 @@ enum EventLoopFutureTestError : Error {
class EventLoopFutureTest : XCTestCase {
func testFutureFulfilledIfHasResult() throws {
let eventLoop = EmbeddedEventLoop()
let f = EventLoopFuture(eventLoop: eventLoop, value: 5, file: #file, line: #line)
let f = EventLoopFuture(eventLoop: eventLoop, value: 5)
XCTAssertTrue(f.isFulfilled)
}
func testFutureFulfilledIfHasError() throws {
let eventLoop = EmbeddedEventLoop()
let f = EventLoopFuture<Void>(eventLoop: eventLoop, error: EventLoopFutureTestError.example, file: #file, line: #line)
let f = EventLoopFuture<Void>(eventLoop: eventLoop, error: EventLoopFutureTestError.example)
XCTAssertTrue(f.isFulfilled)
}

2
Tests/NIOPosixTests/EventLoopTest.swift
View File

@ -1494,7 +1494,7 @@ fileprivate class EventLoopWithPreSucceededFuture: EventLoop {
}
init() {
self._succeededVoidFuture = EventLoopFuture(eventLoop: self, value: (), file: "n/a", line: 0)
self._succeededVoidFuture = EventLoopFuture(eventLoop: self, value: ())
}
func shutdownGracefully(queue: DispatchQueue, _ callback: @escaping (Error?) -> Void) {