search

nim-lang / redis

Official redis wrapper for Nim.
Other
128 stars 35 forks source link

Added an async redis client implementation #4

Closed euantorano closed 7 years ago

euantorano commented 7 years ago

This PR adds an AsyncRedis type. In order for this to work, the API has changed slightly (hence the version bump to 0.2.0) as async procs can't capture varargs or openarrays.

In order to run the test procedures, use nim c -d:testing -d:testasync -r src/redis.nim. In the future I plan to add proper tests that can be ran using nimble test by CI.

It also adds documentation to the docs folder, and fleshes out the readme slightly.

Note that the async version will currently fail with Nim 0.17.0. THis seems to be caused by what I believe is a bug in the Nim stdlib when using asyncnet.recvLine or asyncnet.recvLineInto with unbuffered sockets - I believe these two lines should instead read c = await recvFut as otherwise the receive future may not have completed before the value is read (resulting in the error Error: unhandled exception: Future still in progress.). If anybody can confirm they get this same error, I will open an issue against nim-lang/Nim.

euantorano commented 7 years ago

The copyright header in the source code also probably needs updating, but I'm not sure what you want it updating to. It currently reads the following:

#
#
#            Nim's Runtime Library
#        (c) Copyright 2012 Dominik Picheta
#
#    See the file "copying.txt", included in this
#    distribution, for details about the copyright.
#
dom96 commented 7 years ago

Nice! But please don't commit the doc html files :)

euantorano commented 7 years ago

I'll remove those docs (and the reference from the README) now.

Any thoughts on the possible issue with asyncdispatch.recvLine in the stdlib?

I wouldn't say no to commit access. I don't use pedis a great deal, but do occasionally.

dom96 commented 7 years ago

Huh, weird. I guess unbuffered sockets weren't ever really tested...

What you've written implies that this is a 0.17.0 problem and that a previous version is fine, is that so?

If not then go ahead and create a PR to fix this in the Nim repo.

euantorano commented 7 years ago

I'm not sure if it has ever worked to be honest. I'v only tested against 0.17.0. I'll create a PR in a couple of minutes to patch the issue.

dom96 commented 7 years ago

But hrm, why are you using unbuffered sockets?

euantorano commented 7 years ago

Not sure, the original code was using unbuffered sockets, so I assumed you had a reason at the time :wink:

euantorano commented 7 years ago

Added some tests. The buffered sockets version doesn't need a patch to the stdlib, so this should be good to go. If there's a Nim Circle CI account, would you mind adding this project so that continuous integration will run the tests?

dom96 commented 7 years ago

Done :) https://circleci.com/gh/nim-lang/redis/1