search

Kitura / Kitura-redis

Swift Redis library
Apache License 2.0
95 stars 26 forks source link

Use of callbacks for error handling #10

Closed collinhundley closed 7 years ago

collinhundley commented 8 years ago

I opened PR #9 with the intention of replacing the completion handler in Redis.connect() with more "Swifty" error handling. Upon further inspection, I've come to realize that the use of callbacks is pervasive throughout the entire package.

Can somebody explain why this form of error handling was chosen? It seems crazy to use completion handlers for synchronous operations when a throw would work just as well. Is there something I'm missing, or was this done with some sort of future asynchronous functionality in mind?

I'd love to work on a refactor with better error handling, if there isn't a good reason to keep it the way it is.

shmuelk commented 8 years ago

@collinhundley Thank you for your interest in Kitura-redis

Indeed the idea behind the Kitura-redis APIs is that they should be asynchronous in nature. At the moment the implementation is indeed synchronous in nature. The goal is to get back to this at some future point in time, unless of course we a get a PR on this....

At this point in time what needs to be updated is the Kitura-redis RESP implementation in RedisResp.swift.

collinhundley commented 8 years ago

@shmuelk I'm wondering what the motivation is behind an asynchronous database driver. It's possible that you have a different use-case in mind, but for serving client requests it becomes very cumbersome. Since the response relies on database data, you're essentially forced to block the calling thread anyway until the database query returns. This is a pretty hacky solution that wouldn't be necessary with a synchronous driver. Can you please enlighten me on this design choice?

shmuelk commented 7 years ago

Actually you don't need hack solutions, as RouteHandlers and Kitura middleware are in effect asynchronous in nature. Somewhere in the callback from Redis, in this case, you simply call next().

Having said that, a goal of the Kitura team is to look very hard a some form of Promises.

collinhundley commented 7 years ago

When I originally posted this issue, I was encountering a bug where my RouteHandler would return 400 if the block exited without calling next() or end(). Because of this, I assumed that async operations couldn't be performed in the handlers.

Obviously that's not the case anymore, so this issue isn't as serious as I originally thought. I'm still not a fan of an asynchronous database driver (for code complexity reasons), but clearly there are some benefits to it as well - especially if the database is hosted remotely. I don't see any reason to keep this issue open.