Add request configuration block to allow customizing HTTP request #355
Conversation
|
@garrettmoon you mentioned in the initial PR about locking access to the |
| @@ -281,6 +282,16 @@ - (void)setValue:(nullable NSString *)value forHTTPHeaderField:(nullable NSStrin | |||
| }); | |||
| } | |||
|
|
|||
| - (void)setRequestConfiguration:(PINRemoteImageManagerRequestConfigurationHandler)configurationBlock { | |||
There was a problem hiding this comment.
@zachwaugh you're right about the deadlock.
Here's what I would suggest: We need to add a getter which holds the lock when it accesses requestConfigurationHandler or else it isn't thread safe to access by clients. However, since we know we have the lock in downloadDataWithURL: we can access the ivar directly instead of using the property.
There was a problem hiding this comment.
@garrettmoon sorry for the confusion, but are you saying we do need to a getter, or we don't since we can just use the direct ivar access in downloadDataWithURL:.... I made that direct ivar change and it's the only place this variable is accessed currently.
There was a problem hiding this comment.
I think we do need a getter because the property is exposed publicly? Reading the property externally is currently not thread safe unless I'm mistaken.
There was a problem hiding this comment.
The requestConfigurationHandler property isn't exposed publicly, only the setRequestConfiguration: method.
There was a problem hiding this comment.
Sheesh, I'm doing a terrible job reading this code for some reason. 🤦♂️
| @@ -921,6 +932,10 @@ - (NSURLSessionDataTask *)downloadDataWithURL:(NSURL *)url | |||
| request.allHTTPHeaderFields = headers; | |||
| } | |||
|
|
|||
| if (self.requestConfigurationHandler) { | |||
| request = [self.requestConfigurationHandler(request) mutableCopy]; | |||
There was a problem hiding this comment.
So this should be:
if (_requestConfigurationHandler) {
request = [_requestConfigurationHandler(request) mutableCopy];
}
There was a problem hiding this comment.
Thanks, just pushed that change.
This method is already called within a lock. Locking again will cause a deadlock, so we can use direct access here.
| @@ -281,6 +282,16 @@ - (void)setValue:(nullable NSString *)value forHTTPHeaderField:(nullable NSStrin | |||
| }); | |||
| } | |||
|
|
|||
| - (void)setRequestConfiguration:(PINRemoteImageManagerRequestConfigurationHandler)configurationBlock { | |||
There was a problem hiding this comment.
Sheesh, I'm doing a terrible job reading this code for some reason. 🤦♂️
|
@zachwaugh Thanks so much for adding this and addressing my misguided feedback! |
|
@garrettmoon no problem! |
Replaces #321. I had a similar need to #211, where I want to customize request headers on a per request basis, so I added a request configuration block on PINRemoteImageManager as suggested in that thread. My specific use-case is I want to use PINRemoteImageManager to fetch authenticated images for me, which requires setting an Authorization header with our token. However, I don't want to send this for images to other domains, so I need it to be per request. This seemed like the most straightforward approach, but let me know if you think it should be implemented another way.