Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Clean Up ASAsyncTransaction #trivial #316

Merged
merged 1 commit into from
May 29, 2017
Merged

Conversation

Adlai-Holler
Copy link
Member

  • Make operation values simply id rather than id<NSObject>. id<NSObject> can be a pain to deal with since it can't be implicitly cast, plus it's ugly.
  • Make transactions always call back on the main thread, rather than parameterized callbackQueue.
    • Previously, you couldn't pass any queue for callback other than the main queue anyway.
    • Code was inconsistent. Some places directly used main thread, other places used parameterized callbackQueue.
  • Make operation value atomic since it's set from background thread, but read from main thread.
  • Other misc cleanup.

@ghost
Copy link

ghost commented May 29, 2017

1 Warning
⚠️ Any source code changes should have an entry in CHANGELOG.md or have #trivial in their title.

Generated by 🚫 Danger

@Adlai-Holler Adlai-Holler changed the title Clean Up ASAsyncTransaction Clean Up ASAsyncTransaction #trivial May 29, 2017
Copy link
Member

@appleguy appleguy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Check out my comment before landing - wondering if the queue is right in one place.

Thanks for renovating these core areas of the codebase!

@@ -445,7 +432,8 @@ - (void)addOperationWithBlock:(asyncdisplaykit_async_transaction_operation_block
- (void)addCompletionBlock:(asyncdisplaykit_async_transaction_completion_block_t)completion
{
__weak __typeof__(self) weakSelf = self;
[self addOperationWithBlock:^(){return (id<NSObject>)nil;} queue:_callbackQueue completion:^(id<NSObject> value, BOOL canceled) {
dispatch_queue_t bsQueue = dispatch_get_global_queue(DISPATCH_QUEUE_PRIORITY_DEFAULT, 0);
[self addOperationWithBlock:^(){return (id)nil;} queue:bsQueue completion:^(id value, BOOL canceled) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I bet the (id)nil cast isn't needed anymore.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Surprisingly it is! The compiler inferred nullptr instead of (id)nil when I just put nil. Bizarre I know =/

@@ -445,7 +432,8 @@ - (void)addOperationWithBlock:(asyncdisplaykit_async_transaction_operation_block
- (void)addCompletionBlock:(asyncdisplaykit_async_transaction_completion_block_t)completion
{
__weak __typeof__(self) weakSelf = self;
[self addOperationWithBlock:^(){return (id<NSObject>)nil;} queue:_callbackQueue completion:^(id<NSObject> value, BOOL canceled) {
dispatch_queue_t bsQueue = dispatch_get_global_queue(DISPATCH_QUEUE_PRIORITY_DEFAULT, 0);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

wtf does bsQueue mean? lol

Either way, shouldn't this operation be called on the main queue, if _callbackQueue used to be main? This seems like it might be a behavior change.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

bsQueue means "bullshit queue." The completion will be called on main still, but the bs "work" that we submitted should be done on another queue. If we try to do the work on the main queue, the transaction won't be safe to wait on (the group will get stuck.)

This method isn't currently used and I'd be glad to remove it. Thoughts?

@Adlai-Holler Adlai-Holler merged commit b7cd0b1 into master May 29, 2017
@Adlai-Holler Adlai-Holler deleted the AHCleanupTransaction branch May 29, 2017 23:27
bernieperez pushed a commit to AtomTickets/Texture that referenced this pull request Apr 25, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants