1170382 - Allow interrupting history queries

Status: RESOLVED FIXED

(Firefox for iOS :: Data Storage, defect)

Description

[Wesley Johnston (:wesj)]

We should allow interrupting database queries for the search screen (i.e. if you type quickly).

I don't think we can really be smart about only stopping certain sql with our current setup (single connection). sqlite3_interrupt will kill whatever is running now. Callers get back the error if it happens, and its up to them to re-trigger the call at a future time.

Comment 1

[Wesley Johnston (:wesj)]

Attachment: PR https://github.com/mozilla/firefox-ios/pull/537

https://github.com/mozilla/firefox-ios/pull/537

Comment 2

[Richard Newman [:rnewman]]

Comment on attachment 8613796 [details] [review]
PR https://github.com/mozilla/firefox-ios/pull/537

Having the DB layer call interrupt on every query doesn't feel like the right approach to me.

We don't want to be routinely interrupting; we should be going in the other direction, which is trying to selectively interrupt only when necessary.

Yes, that means exposing the ability to interrupt to code higher up the stack... but that actually seems right to me.

So I would expect SearchLoader to maintain a flag ("am I waiting for results?"), to include the call to interrupt, and I would expect either profile.history or a subclass of Deferred to include the hook that it uses to call it.

Pseudocode for the former:


  class SearchLoader {
    var pending: Bool = false

    ...
    didSet { 
      ...
      if self.pending {
        // Sorry everyone!
        self.history.interrupt()
        self.pending = false
      }
      self.pending = true
      self.history.getSitesByFrecencyWithLimit(...).uponQueue(...)

Pseudocode for the latter:

  class SearchLoader {
    var pending: InterruptibleDeferred<Result<Cursor<Site>>>? = nil

    ...
    didSet { 
      ...
      if let p = self.pending {
        p.interrupt
        self.pending = nil
      }
      let again = self.history.getSitesByFrecencyWithLimit(...)
      self.pending = again
      again.uponQueue(...)



Obviously the latter extends more readily to alternate approaches to canceling.

Comment 3

[Stefan Arentz | :st3fan | ⏰ EST | he/him]

I have a bad feeling about interacting with SQLite at this level. Same with the other patch regarding threading. I can't describe this in technical terms. But it feels wrong that we need this kind of control. I am afraid that this will complicate things and will result in hard to debug problems. Is there a way we can *simplify* our interactions with SQLite instead so that we do not need code like this?

Comment 4

[Richard Newman [:rnewman]]

I do share that concern, which is -- if we do it at all -- I'd like interrupts to be narrowly scoped.

Obviously the best solution is for queries to be (a) fast and (b) debounced, so we never need to interrupt...

Comment 5

[Wesley Johnston (:wesj)]

Moving this up the stack sounds good to me. When I first wrote this stuff, I thought that history operations could return an NSOperation. We could still do something like that I guess with a Deferred/NSOperation hybrid of some sort.

var defered: DeferredOperation
self.defered = history.getSitesByFrecency();
// ... if deferred is still around when another query starts.
self.deferred.cancel()

Maybe I'll try that?

Comment 6

[Wesley Johnston (:wesj)]

Oh, reading above that's pretty similar. Yay!

Comment 7

[Wesley Johnston (:wesj)]

Comment on attachment 8613796 [details] [review]
PR https://github.com/mozilla/firefox-ios/pull/537

This looks more like what I proposed. I wanted to return an NSOperation from the history queries but NSOperation is an abstract class, so instead I made a deferred that looks a bit like an NSOperation.

    let deferred = DeferredSqliteOperation({ (db, err) -> Int? in
      // ... Do some db stuff
      return 1
    }, withDb: myDb).start(onQueue: myQueue)

    // ... Some time later
    deferred.cancel()

or in our case:

  let deferred = history.getSitesByFrecency(...)
  (deferred as? Cancellable).cancel()

Deferred is final so override it isn't... clean. <rant>Final classes and methods are evil.</rant> It also has a few convenience inits() and one private one instead of one with default arguments, which made the Swift compiler angry when I did/didn't override them. I gave up and just edited it after awhile.

Comment 8

[Richard Newman [:rnewman]]

I think this is the right direction. A few comments about connection handling that I think will help us going forward, some nits, and I'm keen to hear a second opinion for a scary change like this!

Comment 9

[Wesley Johnston (:wesj)]

Comment on attachment 8613796 [details] [review]
PR https://github.com/mozilla/firefox-ios/pull/537

Updated with the review comments. I was trying to be a bit careful here about the connection. i.e. a read/write lock around it since this implies its being touched from multiple threads. It feels a bit... fragile :) but I do like it better than exposing a method in SwiftData.

Comment 10

[Stefan Arentz | :st3fan | ⏰ EST | he/him]

Comment on attachment 8613796 [details] [review]
PR https://github.com/mozilla/firefox-ios/pull/537

I think this is good. But I think it is a good idea if rnewman also looked at the sqlite specific pieces.

Comment 11

[Richard Newman [:rnewman]]

Comment on attachment 8613796 [details] [review]
PR https://github.com/mozilla/firefox-ios/pull/537

Two small questions/changes.

I'd land this with a healthy dose of caution: not before a new build goes out, and make sure we get some heavy manual testing!

Comment 12

[Wesley Johnston (:wesj)]

Attachment: PR: https://github.com/mozilla/firefox-ios/pull/586

I was surprised no one mentioned this originally, but now I realize that Deferred is actually an Carthage-imported library (I'd always assumed Third-Party was our dumping ground for real third-party source), so you didn't see that changes :(

I made some changes here so that we can extend Deferred:
1.) Make it non-final
2.) Remove some convenience initializers. Swift was mad when I extended without overriding any initializers. It was also mad when I extended and tried to override a convenience initializer. It was also mad when I made the private initializer public and tried to extend it (it couldn't seem to figure out what I was overriding). For the sake of progress, I gave up and just removed the convenience ones since they weren't doing much that couldn't be done with default arguments anyway (maybe they won't want you to initialize with a value and queue?).

I posted both changes upstream: https://github.com/bignerdranch/Deferred/pulls

Comment 13

[Wesley Johnston (:wesj)]

Reverted this change while we talk about this.

Comment 14

[Wesley Johnston (:wesj)]

Backout: https://github.com/mozilla/firefox-ios/pull/587

Comment 15

[Brian Nicholson (:bnicholson)]

Comment on attachment 8620638 [details] [review]
PR: https://github.com/mozilla/firefox-ios/pull/586

My intuition says we should find another way to do this without having to fork and maintain our own separate repo. I haven't been following/reviewing these PRs, though, so it's not clear to me what your alternatives are. I'll leave the decision up to you and rnewman.

Comment 16

[Wesley Johnston (:wesj)]

Rnewman said this was fine in triage.

I had no idea there was so much hatred for inheritance out there in the world. If this were a protocol, I could kind of get it. But its not and when I made it one things were... messy (Swift doesn't handle generic in protocols very smoothly). If we use composition, we'd have to do things like:

history.clear().deferred ==>>
  history.query().deferred

Comment 17

[Wesley Johnston (:wesj)]

Relanded the original patch now as well.

https://github.com/mozilla/firefox-ios/pull/629