I have a std::map<std::string, SKProduct*> which I populate like this:
// Assume s_map is always accessed in a thread safe way
static auto s_map = std::map<std::string, SKProduct*>{};
-(void)productsRequest:(SKProductsRequest *)request didReceiveResponse:(SKProductsResponse *)response {
auto map = std::map<std::string, SKProduct*>{};
const auto product_id = std::string(
[product.productIdentifier UTF8String]
);
for(SKProduct* product in response.products) {
if(product != nil) {
map[product_id] = product;
[map[product_id] retain];
}
}
s_map = map;
}
Later (when a purchase is made) I find the SKProduct* like this:
auto make_purchase(const product_id_t& product_id) -> void {
// Note that the whole map is copied
const std::map<std::string, SKProduct*> map = s_map;
const auto product_it = map.find(product_id);
if(it == map.end()) {
return;
}
// Here somewhere I get a crash objc_retain_x0
SKProduct* product = product_it->second;
[product retain];
SKPayment* payment = [SKPayment paymentWithProduct: product];
[payment retain];
// Continue the purchase from here on...
}
Am I doing something wrong when storing/retrieving the SKProduct* from the std::map? I'm not familiar with the Objective-C model of reference counting.
(Note that the code is a bit simplified for clarity compared to the original code)
For the question in the title, I would encapsulate this part into a custom container class to wrap the memory management inside. Here is the minimalistic implementation example:
template<typename Key> // Key type must not be a retain-release object
class id_unordered_map {
std::unordered_map<Key, id> m_underlying_map;
void clear() {
for (const auto& element: m_underlying_map) {
[element.second release];
}
m_underlying_map.clear();
}
public:
id_unordered_map() = default;
id_unordered_map(const id_unordered_map& rhs) {
for (const auto& element: rhs.m_underlying_map) {
// makes a shallow copy
m_underlying_map[element.first] = [element.second retain];
}
};
id_unordered_map(id_unordered_map&& rhs) {
for (const auto& element: rhs.m_underlying_map) {
m_underlying_map[element.first] = [element.second retain];
}
rhs.clear();
}
id_unordered_map& operator=(const id_unordered_map& rhs) {
clear();
for (const auto& element: rhs.m_underlying_map) {
// makes a shallow copy
m_underlying_map[element.first] = [element.second retain];
}
return *this;
}
id_unordered_map& operator=(id_unordered_map&& rhs) {
clear();
for (const auto& element: rhs.m_underlying_map) {
m_underlying_map[element.first] = [element.second retain];
}
rhs.clear();
return *this;
}
void setObject(const Key& key, id object) {
removeObject(key);
if (object) {
m_underlying_map[key] = [object retain];
}
}
id getObject(const Key& key) {
if (auto it = m_underlying_map.find(key); it != m_underlying_map.end()) {
return it->second;
} else {
return nil;
}
}
void removeObject(const Key& key) {
if (auto it = m_underlying_map.find(key); it != m_underlying_map.end()) {
[it->second release];
m_underlying_map.erase(it);
}
}
~id_unordered_map() {
clear();
}
};
I suggest a shallow copy approach here, as it's consistent with how Cocoa own collections work. Making a deep copy is considered an exceptional case and requires as separate method (e.g. initWithDictionary:copyItems: constructor of NSDictionary)
However personally I don't feel like there is an apparent mistake in the provided code which makes the app to crash. The error you are observing commonly happens when a message is sent to an object which is not set to nil but already released. And provided no release messages are sent to the objects in the map in between the functions, your SKProduct object has to survive.
Here are a few things to consider though:
make_purchase function is called from. It means, that objects spawned in the delegate thread might go out of autorelease pool they were created in (this, however, should not be a problem provided you retained the objects and no race-condition happens when reading/writing to the map).[SKPayment paymentWithProduct: product]; returns an autoreleased object, which won't expire (at least) until end of the current scope, so you are not required to retain it.clear() it before writing new data to the map.Summing it up, your SKProductsRequestDelegate should looks something like this (the product here is artificial, so i make it on the fly in the response):
NS_ASSUME_NONNULL_BEGIN
@interface TDWObject ()<SKProductsRequestDelegate>
@property (strong, readonly, nonatomic) dispatch_queue_t productsSyncQueue;
@property (assign, nonatomic) id_unordered_map<std::string> products;
@property (strong, readonly, nonatomic) NSMutableSet<SKProductsRequest *> *pendingRequests;
@end
NS_ASSUME_NONNULL_END
@implementation TDWObject
@synthesize products = _products;
#pragma mark Lifecycle
- (instancetype)init {
if (self = [super init]) {
_productsSyncQueue = dispatch_queue_create("the.dreams.wind.property_access.products",
DISPATCH_QUEUE_CONCURRENT);
_pendingRequests = [[NSMutableSet set] retain];
}
return self;
}
- (void)dealloc {
[_pendingRequests release];
[_productsSyncQueue release];
[super dealloc];
}
#pragma mark Properties
- (id_unordered_map<std::string>)products {
__block id_unordered_map<std::string> *data;
dispatch_sync(_productsSyncQueue, ^{
// Take by pointer here, to avoid redundant copy
data = &_products;
});
return *data; // makes a copy for observers
}
- (void)setProducts:(id_unordered_map<std::string>)products {
dispatch_barrier_async(_productsSyncQueue, ^{
_products = std::move(products);
});
}
#pragma mark Actions
- (void)requestProducts {
SKProductsRequest *productRequest = [[SKProductsRequest alloc] initWithProductIdentifiers:[NSSet setWithArray:@[
@"the.dreams.wind.sampleSKU1"
]]];
productRequest.delegate = self;
[productRequest start];
[_pendingRequests addObject:productRequest];
}
- (void)makePurchase {
SKProduct *product = [_products.getObject("the.dreams.wind.sampleSKU1") retain];
// Just checking that the object exists
NSLog(@"%@", product);
[product release];
}
#pragma mark SKProductsRequestDelegate
- (void)productsRequest:(SKProductsRequest *)request didReceiveResponse:(SKProductsResponse *)response {
[_pendingRequests removeObject:request];
decltype(_products) newProducts;
// Artificial Products
for (NSString *identifier in response.invalidProductIdentifiers) {
newProducts.setObject(identifier.UTF8String, [[SKProduct new] autorelease]);
}
self.products = newProducts;
}
You can see here that access/reading of the map is synchronised with use of GCD and Objective-C properties, which, I admit, is horribly ineffective when it comes to C++ objects accessed by value. You will want to optimise it BUT it should work without crashes I believe.
P.S. you usually also want to synchronise reading from/writing to pendingRequests set, but it's not relevant in context of the question, thus I omitted this part.
You may also consider just taking the products arrays by reference, without employing C++ objects, which should work just fine and is much more straightforward:
- (void)productsRequest:(SKProductsRequest *)request didReceiveResponse:(SKProductsResponse *)response {
[_pendingRequests removeObject:request];
NSArray<SKProduct *> *products = [response.products retain];
...
}