search

ra1u / redis-dart

fast redis protocol parser and client
MIT License
84 stars 35 forks source link

Throw Errors received from Redis #8

Closed eknoes closed 5 years ago

eknoes commented 5 years ago

What do you think about throwing errors received from Redis? Otherwise, one has to check the Futures wether they returned an error or the expected data. In my opinion, throwing them directly would be the better way.

ra1u commented 5 years ago

This looks fine for me. It seems that such behavior is expected according to protocol specs.

Can we get one functional test for this feature?

eknoes commented 5 years ago

I can do that, but I don't really have an idea how. As the Future is also returned, in paralell to throwing it. Should we maybe return null and throw the error on error?

When its returned and thrown I don't have an Idea how to write a test.

eknoes commented 5 years ago

BTW: That do you think about using darts test framework? Then, it would be easier with expectLater(fn, throwsArgumentError)

ra1u commented 5 years ago

Thank you for your input.

I think there are few spots to fix synchronous error leaking in library and this PR shows some of current issues with current library. Looking for ideas how to fix synchronous error handling.

eknoes commented 5 years ago

I wrote a test and fixed the synchronous error leak.

ra1u commented 5 years ago

It looks like that performance decreased since v1.1.0 . Can you please compare results between this patch/master and tag v1.1.0 also on your side.

My results:

current master paralel (100 connecetions) 139179 ops/s raw performance 139179 ops/s

tag v1.1.0 paralel (100 connecetions) 165426 ops/s raw performance 174064 ops/s

eknoes commented 5 years ago

I can confirm this :(

current master: parallel(100 connections) with 200000 73341 ops/s raw performance with 200000 74710 ops/s

tag v1.1.0: parallel(100 connections) with 200000 90950 ops/s raw performance with 200000 101781 ops/s

eknoes commented 5 years ago

Looks like performance issues with async are known: https://github.com/dart-lang/sdk/issues/29189

eknoes commented 5 years ago

There is an experimental dart function that we could use that should speed up performance: waitFor. Can you confirm, that this has higher performance: https://github.com/eknoes/redis-dart/tree/experimental-performance

ra1u commented 5 years ago

This works better, but I do not clearly understand how and where changed invariant effects library usage. It have feeling that it should be fine in most cases.

eknoes commented 5 years ago

I think everything works normal, but when an error received it does not return a future, but its more like a:

Future = parseError(stream)
while(Future.notFinished) {
  sleep()
}
throw Error();

So your synchronous program is just slower when you will receive an error, otherwise its a lot more performant. For the use cases I can imagine this is better than "everytime a bit slower".

So I would make a PR, okay?