Swiftify-Corp / Swiftify

42 stars 24 forks source link

@synchronized block results in new dispatch queue instead of `objc_sync_enter` / `objc_sync_exit` #200

Closed amorde closed 2 years ago

amorde commented 2 years ago

Input:

@import Foundation;

@interface SomeObject: NSObject
+ (NSString *)someSharedValue;
@end
@interface SomeObject ()
@end

static NSDictionary *_cachedValues;

@implementation SomeObject: NSObject

+ (NSString *)someSharedValue;
{
    @synchronized(_cachedValues) {
        return [_cachedValues objectForKey:@"SomeKey"];
    }
}

@end

Output:

import Foundation

private var _cachedValues: [AnyHashable : Any]?

class SomeObject: NSObject {
    class func someSharedValue() -> String? {
        let lockQueue = DispatchQueue(label: "_cachedValues")
        lockQueue.sync {
            return _cachedValues?["SomeKey"] as? String
        }
    }
}

Using objc_sync_enter and objc_sync_exit will maintain the previous behavior and also avoid creating a large number of DispatchQueue instances:

import Foundation

private var _cachedValues: [AnyHashable : Any]?

class SomeObject: NSObject {
    class func someSharedValue() -> String? {
        let returnValue: String?
        objc_sync_enter(_cachedValues)
        returnValue = _cachedValues?["SomeKey"] as? String
        objc_sync_exit(_cachedValues)

        return returnValue
    }
}
alex-swiftify commented 2 years ago

Thanks, @amorde for the interesting suggestion! Since this is a relatively easy change, this will be addressed in the converter at the nearest time.

It may take more time for these changes to be included in the Offline version - our macOS desktop developer is in Ukraine and is currently unavailable :(

alex-swiftify commented 2 years ago

P.S. Just wondering if such a solution using defer keyword will be acceptable to you? This might be easier from the code generation standpoint (don't need to introduce a returnValue variable):

import Foundation

private var _cachedValues: [AnyHashable : Any]?

class SomeObject: NSObject {
    class func someSharedValue() -> String? {
        objc_sync_enter(_cachedValues)
        defer { objc_sync_exit(_cachedValues) } 
        return _cachedValues?["SomeKey"] as? String
    }
}
amorde commented 2 years ago

Yeah that makes sense! As long as the code being converted has the return statement inside the @synchronize(objc) block that should work. Other cases would need to be handled differently.

alex-swiftify commented 2 years ago

@amorde I hear what you say about the return statement. We'll see whatever is easier from the implementation standpoint - either your suggestion or mine using the defer { } statement.

alex-swiftify commented 2 years ago

@amorde I just came across this article about pitfalls of using objc_sync_enter / objc_sync_exit. I still think that your suggested changes make sense, but please confirm if you are aware of possible deadlocks with your suggested approach.

alex-swiftify commented 2 years ago

P.S. Fixed (will be available since the next converter update).

Output for your sample:

import Foundation

private var _cachedValues: [AnyHashable : Any]?

class SomeObject: NSObject {
    class func someSharedValue() -> String? {
        objc_sync_enter(_cachedValues)
        defer { objc_sync_exit(_cachedValues) }
        return _cachedValues?["SomeKey"] as? String
    }
}

Another sample (without a return statement inside the @synchronized() block. Input:

@synchronized(myLockObject) {
    NSLog("Hello World");
}

Output:

objc_sync_enter(myLockObject)
print("Hello World")
objc_sync_exit(myLockObject)