infinitered / cdq

Core Data Query for RubyMotion
MIT License
172 stars 35 forks source link

Persistent Background Threads not working as expected #129

Open no-itsbackpack opened 8 years ago

no-itsbackpack commented 8 years ago

I currently have this cdq setup from the wiki to support a persistent background thread

1. cdq.contexts.push(:root)
2. cdq.contexts.push(:main)
3. cdq.contexts.push(:private, named: :network)
4.
5. cdq.contexts.on(:network) do |network_context|
6.  
7.   cdq.save(network_context, always_wait: true)
8.   cdq.save(:main)
9. end

Line 5 throws the following error

context.rb:261:in `on:': expected Proc with 0 arguments (got 1) (ArgumentError)

which comes from this line https://github.com/infinitered/cdq/blob/master/motion/cdq/context.rb#L261. Is there anything I am missing or is the example in the wiki outdated.

I basically want to support a persistent background thread with access to the context being used.

kemiller commented 8 years ago

I think the example is wrong, not sure how that happened. Should be (from memory)

cdq.contexts.on(:network) do 
  cdq.save(:network, always_wait: true)
  cdq.save(:main)
end
no-itsbackpack commented 8 years ago

Ah ok, that makes more sense. so how do I get access to the context to perform an operation like this

1. cdq.contexts.on(:network) do
2.  self.MR_importFromObject(some_object, inContext: :network_context:)
3.  predicate = self.where('NOT(category IN %@) and default = %@', selected_categories, false)
4.  predicate.array.each { |f| f.destroy }
4.  cdq.save(:network, always_wait: true)
5.  cdq.save(:main)
6.end

How do I get access to the network context in the block to perform the action on Line 2. Do I just call context ?

Also line 4 yields the following error

targeted_query.rb:47:in `array': No context has been set.  Probably need to run cdq.setup (RuntimeError)`

Even though I have already pushed the appropriate contexts onto the stack

kemiller commented 8 years ago

Right, the context isn't on the stack at that point... there's definitely a piece of the puzzle missing here. What behavior would you naively expect? Seems like it should have pushed the context in question onto the stack for you.

no-itsbackpack commented 8 years ago

hmm... So my expectation was to setup my stack like below once and it would last the lifespan of the app

1. cdq.contexts.push(:root)
2. cdq.contexts.push(:main)
3. cdq.contexts.push(:private, named: :network)

I run the code above in my AppDelegate before setting the RootViewController. The piece of code that errors out runs in one of my view controllers, so my expectation was that the context should have been already pushed onto the stack by then. Am I wrong in assuming this ?

OR do I have to manually push a new network context onto the stack each time I want to perform background saves ?

kemiller commented 8 years ago

The stack exists per-thread and always has. For a private thread, you want it to do its work and complete, and let the main thread save down to the root store when it makes sense for the UI. But since I designed it that way, I've changed how the save actually happens in a few ways, so it's possible I can set the whole stack up by default on new threads. I'll have to think about that. But at minimum the context associated with the private thread itself should be there.

no-itsbackpack commented 8 years ago

I placed a couple debug statements right before the line that errored out and my debug statements do show cdq.contexts.all containing all three contexts, presumably the contexts I pushed onto the stack in AppDelegate. I believe cdq.context.on(:network) should have errored out if it did not exist on the stack, no ? Totally confused at this point... :persevere:

no-itsbackpack commented 8 years ago

Just changed my code to use

cdq.background do 
  # run code
  cdq.save(always_wait: true)
  cdq.save(:main)
end

and everything seems to work fine. Probably going to stick with this approach for obvious reasons. Thanks for helping me out @kemiller :+1: :+1: :+1:

spnkr commented 8 years ago

how does this compare to NSAsynchronousFetchRequest, e.g.

id = 27342
fetchRequest = NSFetchRequest.alloc.initWithEntityName('Post')
fetchRequest.predicate = NSPredicate.predicateWithFormat("id==%@",id)
nsaCompletion = Proc.new do |result|
  if result.finalResult.count == 0
    po = Post.find_or_create_by_id(id,withProperties:props) #this line makes a new post object
  else
    po = result.finalResult.first
  end
end
asyncFetch = NSAsynchronousFetchRequest.alloc.initWithFetchRequest(fetchRequest, completionBlock:nsaCompletion)
error_ptr = Pointer.new(:object)
cdq.contexts.all.last.executeRequest(asyncFetch,error:error_ptr)
dchersey commented 7 years ago

So, I am having similar issues .... and I suspect I am misunderstanding how the stack is set up for different threads. I am doing

    cdq.contexts.push(:main)
    cdq.contexts.create(:private, named: :map)

all on the main thread.

When my network callback occurs using AFNetworking, I attempt to store the data using the map context, which is monitored by the map view for changes. This way I can efficiently update my map in the background by watching core data events (using an NSFetchedResults controller). I have the same need with TableScreens and am using DataTableScreen from redpotion.

In my network callback block I have:

mp cdq.contexts.all

This shows 2 contexts on the stack. I am still on the main thread here.

Then I do

cdq.contexts.on :map do 
   mp cdq.contexts.current
   mp cdq.contexts.all
    store_my_stuff
end

but I get an targeted_query.rb:47:inarray': No context has been set mp cdq.contexts.current' in this block returns the correct context but mp cdq.contexts.all returns [] at this point.

This block is of course not on the main thread, because I am executing it on the private context queue I set up. But inside that block, cdq.contexts.current is nil and this is why it is raising the exception.

Any ideas? cdq.background do appears to work, and the difference is it pushes a new private context, vs creating one.

dchersey commented 7 years ago

Ok, an update ... I have managed to get this to work by emulating whatcdq.background does. First, I get the context

context = cdq.contexts.map

Then, I put myself 'on' the context -- for those who are not clear, this causes the block to run ON the context's private thread. Wish I could have known that off the top -- contexts must always be accessed from the same thread and this is how you do it.

But in this strange case, possibly due to it being on a network request that occurs almost immediately on app launch, I need to also push the context. Pushing with a block causes the context to be pushed only for that block; it is popped immediately after. I am on the main thread here; as NSThread.currentThread.isMainThread confirms, and cdq is clearly already set up in my app delegate as the first line of code, so I have no idea what is going on. It is possible that I'm on some kind of operation queue as the code in question is being called as part of the MKMapDelegate protocol, when the user location is resolved.

cdq.contexts.on context do 
   cdq.contexts.push context do 
       mp cdq.contexts.current
       mp cdq.contexts.all
       store_my_stuff
   end
end

Also to note, in this situation, the 'stuff' is not saved to the persistence store! No amount of cdq.save(:map), cdq.save, seems to cause that to happen. Why? because clearly the cdq stack visible in this operation is not the one that I set up. Why? I have no idea, open to suggestions.

To work around that, I have done a

Dispatch::Queue.main.async do
    cdq.save
end

That does work, saving the main cdq stack.

I have other code, in another place, executed from the main thread where

cdq.contexts.on context do 
   query_and_do_some_UI_stuff
end

does work as expected.

Mystified, but ok for now.

kemiller commented 7 years ago

Having to separately save the main stack is precisely what you are supposed to do in this case. If you look at the last pattern documented here you can see that on doesn't actually set up a stack, it just runs code on the private thread for that context. Because the thread is persistent, I'm pretty sure you can set up a stack once and it will hang out, which is why it doesn't automatically push for you. I prefer the method used in the example where you save named contexts explicitly since it's clearer what's happening. If I had to do CDQ over again I might not use the stack at all and just use named contexts, in fact, because you really do have to think about what you're doing in a threaded environment.

kemiller commented 7 years ago

Oh and to be extra clear: stacks are per-thread. Every time you switch threads you have to set it up, or used named contexts everywhere.

dchersey commented 7 years ago

That makes sense ... what threw me about that example was that the :network thread was pushed during setup, which implied to me that it was part of the stack, and in fact the current context, even though it's private and intended only for the network saves.

So I did a create(:private, named :map) and that made the parent context of the new thread the :main context (which I felt should make it part of the stack) and then did an :on when I wanted to use it. But in that :on block, the stack was not initialized.

kemiller commented 7 years ago

Yeah, the parent/child relationship is totally separate from the stack, by design. Could do a better job of explaining that.

dchersey commented 7 years ago

It's a great framework, and now that I have separated out the issues it all seems very straightforward. I hope this thread helps others too.

I still am not sure why the cdq stack is empty when I get to the above code but not when I execute the later code (also from a map notification). Since the stack is stored in class variables, and those are not threadsafe, could there be a timing issue I need to track down?

kemiller commented 7 years ago

Yes, the first time you use that thread there will be no stack, but the second time it will still be there. The stack is stored in thread-local storage.

jr180180 commented 7 years ago

Awesome thread here, gentlemen! Really helpful information here.

dchersey commented 7 years ago

Great, thank you. That makes sense ... I suppose I could just do a

cdq.contexts.create(:private, :blah)
cdq.contexts.on :blah do 
  cdq.contexts.push :blah {}
end

and that would establish the stack; next time just do

cdq.contexts.on :blah do 
   ...
end

Is that correct?

kemiller commented 7 years ago

Yup.