Don't crash in Statement.next.

[nickmshelley]

No description provided.

[nickmshelley]

We're getting a lot of crashes in Statement.next() with unhelpful crash logs. I wanted to make it throws but that doesn't match the signature of the IteratorProtocol method it's implementing. This isn't my favorite solution, but I don't like crashes and I'm not sure what else to do. I would love some feedback and/or ideas about this.

[stephanheilner]

This is definitely better than force unwrapping. The error message could maybe be improved, but the concept is good. Throw errors, don't crash

[stephanheilner]

How hard would it be to have next() throw the error instead of swallowing it here with a print?

[jberkel]

This is not a good solution. Slightly better would be to use precondition here so the check can be disabled with -Ounchecked if desired. It would still not solve the actual problem though, which requires bigger changes.

What kind of errors do you see in your app? How does it recover from them?

[stephanheilner]

You think force unwrapping and crashing is better?

[jberkel]

@stephanheilner Obv not ideal, but still better than ignoring errors.

[nickmshelley]

I agree that swallowing the error isn't ideal. I liked the assert idea. (I don't feel like it's really a precondition at that point since the work has already been done, but I can change it if you want. Assertions can also be turned off but I think it's a different flag.)

A couple other options I thought of are to not conform to IteratorProtocol and just make next throw, or change the return type to be some Result type that can include an error. The downside of both of those is that they won't be backwards compatible though. The assert/precondition approach can at least give users the option to not crash by turning them on or off.

[groue]

Did you consider that Sequence may not be the correct type?

Hey, @nickmshelley just had the same idea :-) You may be interested in the Cursor protocol:

• pitch on swift-evolution (may well deserve a little support)
• Cursor.swift

GRDB's cursors share traits with lazy sequences and iterators:

let cursor = /* some cursor of Row, for example */
while let row = try cursor.next() { ... }
try cursor.forEach { row in ... }
try cursor
    .filter { row in ... }
    .map { row in ... }...

[nickmshelley]

@jberkel I just realized I never answered your question. The main problem is that we don't see crashes when running locally very often, but when we do we get a fairly helpful message from the error (like "Disk IO error") so we can try to prevent it in the future.

However, the crash reports from Fabric give stack traces and messages that aren't as helpful (see attached image). All crashes that happen in Statement.next() have similar beginning stack traces, but when looking through them the rest of the stack varies among all of the places we use SQLite.swift in our app (or libraries we include that use it), some of which take user input for FTS search, so it's nearly impossible to tell what went wrong with the information we have. And since that is the top crasher in our app and the next top one happens an order of magnitude less, I'd really like to get this figured out. I'm willing to do the bigger changes required to correctly solve this problem if you aren't ok with this assert/precondition stopgap.

[jberkel]

Yes, looks like Sequence / IteratorProtocol does not help here and we need to introduce another type. I just checked my own project which uses SQLite.swift and the pattern employed is almost always:

return try connection.prepare(query).map { mapRow($0) }

I imagine that providing a few simple (throwing) operations such as map and reduce would handle the majority of use cases.

Interesting question is API evolution, for now we could keep the current state and return a new type explicitly:

return try connection.prepare(query).rowCursor.map { mapRow($0) }

This would make it opt-in, and a future major release of the library could make this the default.

@groue surprised nobody replied to your post on the swift mailinglist, I'd imagine this to be a more common problem. Sometimes I feel that mundane things like error handling get ignored in this pure functional developer view of the world.

But it occurred to me that instead of throwing errors we could just return a new type which encapsulates the error:

enum RowResult {
  case error(SQLite.Result)
  case row(RowData)
}

public func next() -> RowResult? {
}

This would lend itself to composition so that you could chain multiple operations via flatMap. A related universal Either type has already been suggested on swift-evolution but the proposal was not accepted.

The client code could then use the result type returned by the library and handle it internally or simply rethrow the error (one could image adding a set of methods which fail fast, to emulate current behaviour).

One could also look at it from a reactive programming perspective, where iterators are reversed into Observables (or Signals) which emit events:

Event is an enumerated type representing either a value or one of three terminal events:

• The value event provides a new value from the source. [...]
• The failed event indicates that an error occurred before the signal could finish. [...]
• The completed event indicates that the signal finished successfully [...]

(RAC events)

Not explicitly throwing errors in our own code would facilitate this programming model as well.

[groue]

@jberkel RowResult (or a Row wrapped in a generic Result or Either type) is an interesting idea.

May I brainstorm with you a little bit?

I guess that Result has serious consequences on client code, that you will eventually evaluate. I can't say if forcing users into mapping/flatMapping/unwrapping results is a good idea or not - it depends on how much "functional" you want SQLite.swift to look and feel. Besides, sequences of results can not be turned into lazy sequences of unwrapped userland types: the memory-efficient consumption of SQLite statements that return many rows would require users to use Result:

let lazySequenceOfRowResults = try connection.prepare(query)

let arrayOfUserResults = lazySequenceOfRowResults
    .map { $0.map { User(row: $0) } }
    
let lazySequenceOfUserResults = lazySequenceOfRowResults
    .lazy
    .map { $0.map { User(row: $0) } }
    
let userArray = try lazySequenceOfRowResults
    .map { try User(row: $0.unwrap()) }
    
let lazySequenceOfUsers = /* impossible */

// Consumption of result sequence:
for userResult in lazySequenceOfUserResults {
    switch userResult {
    case .success(let user): print(user.name)
    case .failure(let error): ...
    }
}

Cursors, on the other hand, have the advantage of being always lazy, and to be able to wrap any type, including userland types. This means that they don't make users pay with functional idioms when they want a memory-efficient consumption of SQLite statements that return many rows:

// With GRDB
let users = try User.filter(...).fetchCursor(db)  // DatabaseCursor<User>
while let user = try users.next() { // or try users.forEach { user in ... }
    print(user.name)
}

Applied to SQLite.swift, this would read:

let rows = try connection.prepare(query).cursor // Cursor of Row
let users = rows.map { User(row: $0) }          // Cursor of User
let userArray = try Array(users)                // Array of User

// Consumption of cursor
while let user = try users.next() { // or try users.forEach { user in ... }
    print(user.name)
}

As for support for reactive programming, some fast thoughts: cursors need glue code to be turned into observables that can feed a Reactive library. But sequences of SQLite.Result would need some glue code as well. We don't escape glue code in both cases. I don't known which one is easier to write, though.

[groue]

@jberkel The cursor API has another advantage:

// A sequence of Result is designed for individual error handling:
for result in results {
    switch result {
    case .success(let value): ...
    case .failure(let error):
        // Is it the last step? Will I get more successes? More failures?
        fud()
    }
}

// Cursor API is designed to stop on first error - just like SQLite statements:
try {
    while let value = try values.next() { // or try values.forEach { value in ... }
        ...
    }
} catch {
    // Fetch has eventually failed. No more success. No more failures.
}

[nickmshelley]

You could actually consider that last point a disadvantage to using cursers, depending on how you look at it @groue. The result approach gives users the most control: if they want to abort on first failure like the cursor approach does by default, they can, but if they want to continue they can as well (but not with cursors as they are currently implemented).

Of course, maybe the extra control isn't worth the extra complexity, and I think aborting on the first failure is a very sensible default, even if it could be considered limiting.

@jberkel I think either the backwards-incompatible result type approach or the opt-in (at least for now) row cursor approach are both fine, both having their own advantages and disadvantages. I don't know enough about RAC to properly evaluate your last suggestion, but I'd be hesitant to force RAC on users of the library.

Let me know which direction you want me to proceed. Or if you're inclined to just implement whatever you have in mind yourself, I'm perfectly fine with that as well. 😉

[groue]

You could actually consider that last point a disadvantage to using cursers, depending on how you look at it @groue. The result approach gives users the most control: if they want to abort on first failure like the cursor approach does by default, they can, but if they want to continue they can as well (but not with cursors as they are currently implemented).

Of course, maybe the extra control isn't worth the extra complexity, and I think aborting on the first failure is a very sensible default, even if it could be considered limiting.

Good points.

Now if we speak precisely: once sqlite3_step() has returned an error, a statement can not be reused (sqlite3_step, sqlite3_reset, etc. all return the same error ad nauseam).

So it's not a gratuitous limitation or an opinionated choice: it's actual API design.

[nickmshelley]

Now if we speak precisely: once sqlite3_step() has returned an error, a statement can not be reused (sqlite3_step, sqlite3_reset, etc. all return the same error ad nauseam).

Thanks for pointing that out, I didn't know that. Pretty much makes my point moot then. 😄

I'm not familiar at all with the internal workings of SQLite, so I'm glad there are people around who are and are willing to make libraries that abstract that away for the rest of us.

[nickmshelley]

Implemented in #647.