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

fix block may not be called #78

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

xinjixjz
Copy link

Block may not be called when the user sends message to TMCache
instance, which will cause some problems.

Block may not be called when the user sends message to TMCache
instance, which will cause some problems.
@irace
Copy link
Contributor

irace commented Feb 28, 2015

At first glance this doesn’t look like something we’d want to add. Can you explain what problems this fixes?

@xinjixjz
Copy link
Author

xinjixjz commented Mar 1, 2015

  1. Theoretically, we should call the block in every condition branches, because we should notify caller that the work has been done;
  2. If user's some requirements depend on setObject:forKey:block block callback (but object or key is nil), then the block will never be called, which will cause some problems.
  3. For example:
- (void)deadLockMethod
{
    __block id objectForKey = nil;
    dispatch_semaphore_t semaphore = dispatch_semaphore_create(0);

    // user should wait the signal, but the key is nil and then block will never be called.
    [[TMCache sharedCache] objectForKey:nil block:^(TMCache *cache, NSString *key, id object) {
        objectForKey = object;
        dispatch_semaphore_signal(semaphore);
    }];

    dispatch_semaphore_wait(semaphore, DISPATCH_TIME_FOREVER);
}

@irace
Copy link
Contributor

irace commented Mar 3, 2015

Passing a nil input sounds like programmer error to me. I think it’s something that should be checked for in userspace using assertions, rather than something that the library includes code to explicitly support.

Additionally, your example above is a bit too contrived to be compelling, as you wouldn’t use a semaphore to synchronize TMCache’s asynchronous APIs when it already provides synchronous alternatives for you.

@xinjixjz
Copy link
Author

xinjixjz commented Mar 4, 2015

It is just a example, my point is library should consider the exception input, and the asynchronous APIs should notify caller in all conditions including exception conditions.
Additionally, in the synchronous APIs, the key and object will be set to nil in other thread and block will never be called, which will cause deadlock.
Any way, I think a robust library should consider exception input!

@lhyhsx
Copy link

lhyhsx commented May 15, 2015

@xinjixjz 这个提交挺好啊!他不给你合并,可惜了。

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.

3 participants