mac: Migrate HTTPTransportMac to NSURLSession [crashpad/crashpad : main]
Justin Cohen (Gerrit)
Justin Cohen voted and added 1 comment![]( "Open in Gerrit")
Votes added by Justin Cohen
| Commit-Queue | +1 |
1 comment
I'm going through my backlog. Do we want to fix this?
Related details
- Mark Mentovai
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Mark Mentovai (Gerrit)
Mark Mentovai added 2 comments![]( "Open in Gerrit")
NSURLSession* session = [NSURLSession sharedSession];Do we want the shared session or a `-sessionWithConfiguration:[NSURLSessionConfiguration ephemeralSessionConfiguration]`?
dispatch_semaphore_signal(semaphore);The semaphore in the completion handler seems like an awful way to make this synchronous. Do we have nicer sugar around this? This can’t be the only place this pattern has come up.
Otherwise: `#include <dispatch/dispatch.h>`.
Related details
- Justin Cohen
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Justin Cohen (Gerrit)
Justin Cohen added 2 comments![]( "Open in Gerrit")
NSURLSession* session = [NSURLSession sharedSession];Do we want the shared session or a `-sessionWithConfiguration:[NSURLSessionConfiguration ephemeralSessionConfiguration]`?
Ah, ephemeralSessionConfiguration is a good idea. Good catch. I don't think we want a shared session caching things, especially on iOS when we are running in the host app.
Also added finishTasksAndInvalidate
dispatch_semaphore_signal(semaphore);The semaphore in the completion handler seems like an awful way to make this synchronous. Do we have nicer sugar around this? This can’t be the only place this pattern has come up.
Otherwise: `#include <dispatch/dispatch.h>`.
util/ios/scoped_background_task.mm uses a similar dispatch_semaphore_signal/dispatch_semaphore_wait pattern.
I could use crashpad::Semaphore semaphore(0) / semaphore_ptr->Signal(); / semaphore.Wait();, but it's unclear if that's any better. it also means capturing the semaphore_ptr, but since we are waiting it doesn't really matter.
any preference? For now added the missing include.
Related details
- Mark Mentovai
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Mark Mentovai (Gerrit)
Mark Mentovai added 3 comments![]( "Open in Gerrit")
dispatch_semaphore_t semaphore = dispatch_semaphore_create(0);Call `dispatch_release` on me when you’re done!
dispatch_semaphore_signal(semaphore);Justin CohenThe semaphore in the completion handler seems like an awful way to make this synchronous. Do we have nicer sugar around this? This can’t be the only place this pattern has come up.
Otherwise: `#include <dispatch/dispatch.h>`.
util/ios/scoped_background_task.mm uses a similar dispatch_semaphore_signal/dispatch_semaphore_wait pattern.
I could use crashpad::Semaphore semaphore(0) / semaphore_ptr->Signal(); / semaphore.Wait();, but it's unclear if that's any better. it also means capturing the semaphore_ptr, but since we are waiting it doesn't really matter.
any preference? For now added the missing include.
util/ios/scoped_background_task.mm uses a similar dispatch_semaphore_signal/dispatch_semaphore_wait pattern.
I could use crashpad::Semaphore semaphore(0) / semaphore_ptr->Signal(); / semaphore.Wait();, but it's unclear if that's any better. it also means capturing the semaphore_ptr, but since we are waiting it doesn't really matter.
any preference? For now added the missing include.
It’s the same. But it would have made it easier to do the right thing (automatic `dispatch_release`, it checks relevant return values.)
I was hoping that there was a cleaner pattern for this, but maybe not. OK.
[session finishTasksAndInvalidate];I would have hoped that it did something like this in its `-dealloc`, but OK.
Related details
- Justin Cohen
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Justin Cohen (Gerrit)
Justin Cohen added 2 comments![]( "Open in Gerrit")
dispatch_semaphore_signal(semaphore);Justin CohenThe semaphore in the completion handler seems like an awful way to make this synchronous. Do we have nicer sugar around this? This can’t be the only place this pattern has come up.
Otherwise: `#include <dispatch/dispatch.h>`.
Mark Mentovaiutil/ios/scoped_background_task.mm uses a similar dispatch_semaphore_signal/dispatch_semaphore_wait pattern.
I could use crashpad::Semaphore semaphore(0) / semaphore_ptr->Signal(); / semaphore.Wait();, but it's unclear if that's any better. it also means capturing the semaphore_ptr, but since we are waiting it doesn't really matter.
any preference? For now added the missing include.
util/ios/scoped_background_task.mm uses a similar dispatch_semaphore_signal/dispatch_semaphore_wait pattern.
I could use crashpad::Semaphore semaphore(0) / semaphore_ptr->Signal(); / semaphore.Wait();, but it's unclear if that's any better. it also means capturing the semaphore_ptr, but since we are waiting it doesn't really matter.
any preference? For now added the missing include.
It’s the same. But it would have made it easier to do the right thing (automatic `dispatch_release`, it checks relevant return values.)
I was hoping that there was a cleaner pattern for this, but maybe not. OK.
Er, I thought we don't need this since we build with ARC.
error: 'release' is unavailable: not available in automatic reference counting mode
269 | dispatch_release(semaphore);
| ^
[session finishTasksAndInvalidate];I would have hoped that it did something like this in its `-dealloc`, but OK.
- Mark Mentovai
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Mark Mentovai (Gerrit)
Mark Mentovai voted and added 2 comments![]( "Open in Gerrit")
Votes added by Mark Mentovai
| Code-Review | +1 |
2 comments
dispatch_semaphore_t semaphore = dispatch_semaphore_create(0);Call `dispatch_release` on me when you’re done!
Oh, I forgot that dispatch objects via the C interface participated in ARC.
dispatch_semaphore_signal(semaphore);Justin CohenThe semaphore in the completion handler seems like an awful way to make this synchronous. Do we have nicer sugar around this? This can’t be the only place this pattern has come up.
Otherwise: `#include <dispatch/dispatch.h>`.
Mark Mentovaiutil/ios/scoped_background_task.mm uses a similar dispatch_semaphore_signal/dispatch_semaphore_wait pattern.
I could use crashpad::Semaphore semaphore(0) / semaphore_ptr->Signal(); / semaphore.Wait();, but it's unclear if that's any better. it also means capturing the semaphore_ptr, but since we are waiting it doesn't really matter.
any preference? For now added the missing include.
Justin Cohenutil/ios/scoped_background_task.mm uses a similar dispatch_semaphore_signal/dispatch_semaphore_wait pattern.
I could use crashpad::Semaphore semaphore(0) / semaphore_ptr->Signal(); / semaphore.Wait();, but it's unclear if that's any better. it also means capturing the semaphore_ptr, but since we are waiting it doesn't really matter.
any preference? For now added the missing include.
It’s the same. But it would have made it easier to do the right thing (automatic `dispatch_release`, it checks relevant return values.)
I was hoping that there was a cleaner pattern for this, but maybe not. OK.
Er, I thought we don't need this since we build with ARC.
error: 'release' is unavailable: not available in automatic reference counting mode
269 | dispatch_release(semaphore);
| ^
The only thing left to do here is maybe `CHECK` on `dispatch_semaphore_create`’s return value.
Related details
- Justin Cohen
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Justin Cohen (Gerrit)
Justin Cohen added 2 comments![]( "Open in Gerrit")
NSURLSession* session = [NSURLSession sharedSession];Justin CohenDo we want the shared session or a `-sessionWithConfiguration:[NSURLSessionConfiguration ephemeralSessionConfiguration]`?
Ah, ephemeralSessionConfiguration is a good idea. Good catch. I don't think we want a shared session caching things, especially on iOS when we are running in the host app.
Also added finishTasksAndInvalidate
Acknowledged
dispatch_semaphore_signal(semaphore);Justin CohenThe semaphore in the completion handler seems like an awful way to make this synchronous. Do we have nicer sugar around this? This can’t be the only place this pattern has come up.
Otherwise: `#include <dispatch/dispatch.h>`.
Mark Mentovaiutil/ios/scoped_background_task.mm uses a similar dispatch_semaphore_signal/dispatch_semaphore_wait pattern.
I could use crashpad::Semaphore semaphore(0) / semaphore_ptr->Signal(); / semaphore.Wait();, but it's unclear if that's any better. it also means capturing the semaphore_ptr, but since we are waiting it doesn't really matter.
any preference? For now added the missing include.
Justin Cohenutil/ios/scoped_background_task.mm uses a similar dispatch_semaphore_signal/dispatch_semaphore_wait pattern.
I could use crashpad::Semaphore semaphore(0) / semaphore_ptr->Signal(); / semaphore.Wait();, but it's unclear if that's any better. it also means capturing the semaphore_ptr, but since we are waiting it doesn't really matter.
any preference? For now added the missing include.
It’s the same. But it would have made it easier to do the right thing (automatic `dispatch_release`, it checks relevant return values.)
I was hoping that there was a cleaner pattern for this, but maybe not. OK.
Mark MentovaiEr, I thought we don't need this since we build with ARC.
error: 'release' is unavailable: not available in automatic reference counting mode
269 | dispatch_release(semaphore);
| ^
The only thing left to do here is maybe `CHECK` on `dispatch_semaphore_create`’s return value.
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Mark Mentovai (Gerrit)
Mark Mentovai voted and added 1 comment![]( "Open in Gerrit")
Votes added by Mark Mentovai
| Code-Review | +1 |
- Justin Cohen
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Justin Cohen (Gerrit)
Justin Cohen voted Commit-Queue+2![]( "Open in Gerrit")
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
crashpad-scoped@luci-project-accounts.iam.gserviceaccount.com (Gerrit)
crashpa...@luci-project-accounts.iam.gserviceaccount.com submitted the change![]( "Open in Gerrit")
Change information
mac: Migrate HTTPTransportMac to NSURLSession
Replaces the deprecated NSURLConnection API with NSURLSessionDataTask
and a dispatch semaphore to maintain the synchronous
ExecuteSynchronously interface contract. Uses an ephemeral session
configuration to avoid persisting caches or cookies to disk, and
invalidates the session object upon task completion to prevent resource
leaks.
Since the minimum deployment target is macOS 12.0, NSURLSession is
guaranteed to be available at runtime.
- M util/net/http_transport_mac.mm
Code-Review: +1 by Mark Mentovai
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |