polqf / RealmResultsController

A NSFetchedResultsController implementation for Realm written in Swift
MIT License
188 stars 25 forks source link

Update Realm dependency to 0.97 #52

Closed polqf closed 8 years ago

polqf commented 8 years ago

:tophat: What? Why?

polqf commented 8 years ago

UPDATE: This PR is not ready yet, Realm added on the 0.97 a check when adding a notification block (addNotificationBlock(block:)). This has to occur within runloops.

polqf commented 8 years ago

Ended up doing the same hackish solution done in https://github.com/Roobiq/RBQRealmNotificationManager/commit/23fa5777406112bff248407d69cecd1450560e43

bigfish24 commented 8 years ago

@poolqf here is my better way around the limitation: https://github.com/Roobiq/RBQRealmNotificationManager/commit/6a3a5d38c13215682b7925f1b00a961ae5855a8a

bigfish24 commented 8 years ago

@poolqf also, my understanding is this is a proper solution because the notification is only being registered on the same thread as the transaction. When you register for a change notification on another thread or in a different process the notification occurs through a named pipe which requires that the runloop is attached to the thread and is running (all Cocoa created threads have a runloop but only the main thread has the run loop running already). However, when you are simply trying to get a notification on the same thread as we are here, then this doesn't use a named pipe but simply synchronously with the commit calls any notification blocks registered to the Realm on that thread.

So the short answer is, while this might seem hacky, it is appropriate in this use-case specifically and arguably Realm's check should not throw an exception around runloops when attempting to register a notification on the same thread.

polqf commented 8 years ago

@bigfish24 first of all thanks for taking you time on explaining it so well. To be honest, I did not have any idea to solve this problem, since Realm released the 0.97 I've been trying different things with no luck. Then I found your GH issue, and I've been following it. The reason why I was treating the workaround as hack was that in you code there was a comment saying that, but sorry if that offended you.

As for the workaround itself, even that the first one made a lot of sense for me, the second one seems to be more accurate, the while on the first was scary. But there's a thing that I don't get in the second one, you are stopping the runloop after the block is executed. As fas as I know, CFRunLoopPerformBlock's block is going to be enqueued, and not executed synchronously. That makes me think that the CFRunLoopStop(CFRunLoopGetCurrent()); could create some kind of problems, and the CFRunLoopRun() is not going to change anything.

What am I getting wrong? 😅

bigfish24 commented 8 years ago

@poolqf you didn't offend at all, the first attempt was hacky and I hadn't yet become comfortable this was appropriate.

The second version is cleaner since the block is queued for the next loop of the run loop. It doesn't ever run though because the run loop isn't actually running yet. That's why you have to tell the run loop to run which will execute the block. You have to then stop the run loop though so that further code can execute (when you call CFRunLoopRun() the thread is "stuck" looping in the run loop, which means the checkToken method won't return until you stop the run loop).

polqf commented 8 years ago

@bigfish24 Wow, another time, big thanks for the explanation. That totally makes sense!

bigfish24 commented 8 years ago

No problem, happy to help! Initially I thought Realm could prevent the exception for this unique use case but from Realms perspective it can't guarantee that the notification block is simply there to listen to this threads notification and no others hence why there is a check for a runloop. Given that we know this to be the case ourselves, getting around the check is appropriate.

mariabernis commented 8 years ago

:+1:

fegabe commented 8 years ago

:+1:

isaacroldan commented 8 years ago

:+1:

isaacroldan commented 8 years ago

:shipit:

isaacroldan commented 8 years ago

Thanks @bigfish24 for explaining how everything works, TIL!