Upgrade hiredis to v1.2.0

[nicolasff]

Hi Michal,

Thanks for the suggestion, I'll look into it. As you can see from the diff size from a simple file copy (+5,418/-510 😳) and the large number of files added rather than modified, the hiredis files in the Webdis repo are not just a full copy of the hiredis repo.

Let me take a look at what's new first to make sure this doesn't break anything as a side-effect.

[nicolasff]

Update, I've created a branch with Hiredis v1.20 if you want to give it a try: https://github.com/nicolasff/webdis/tree/hiredis-1.2.0

Here's how I imported the files:

for f in $(find $hiredis -type f | grep -vw examples | grep -v travis.yml | grep -vEw '\.git|\.github|fuzzing|appveyor' ); do \
  fh=$(echo $f | cut -f 6- -d /) # the "6" here depends on the depth of $hiredis
  echo $fh
  cp $hiredis/$fh ./src/hiredis/$fh
done

After that I still removed unused files like CMakeLists.txt or related .in files.

The main idea is to import source files only, not examples or test files which would be unused. I always keep the license file, of course.

There are also a couple of changes that need to be made to the Hiredis source code to avoid build warnings. For this I re-imported previous changes from 05f168f and fc67109 as a single commit: 7d7d3f72a7e31c31d3548d04a8f8a06e2771f271.

I'll go over the changes more carefully to see if anything can be exposed, like when https://github.com/redis/hiredis/commit/011f7093c001b6584252418fc9f657c374bdca66 was backported as 4350a051ddfd2da454d5fe6070330265025f740c.

[MichalPopovic]

Hi Nicolas, this pull request was created by mistake, I just wanted to merge to my fork first and then fully tested it and open pull request after. My intention is use new function from version v1.2.0.

After v1.2.0 we modified how we invoke poll(2) to wait for connections to complete, such that we will now retry the call if it is interrupted by a signal until: a) The connection succeeds or fails. b) The overall connection timeout is reached.

Missing retry causes a missing data for elasticcache when bandwith of cluster is exceeded by other service.

I have run all regression tests sucessfully and now I am testing it in pre-production environment. I will let you know my findings.

[nicolasff]

Ah yes, I see the commits about poll(2) between v1.2.0 and the current master.

But then I don't really understand why you'd want Webdis to use Hiredis v1.2.0 if these changes were made later and are not in the 1.2.0 release. If in fact you do want to test on v1.2.0 I'd encourage you to use the https://github.com/nicolasff/webdis/tree/hiredis-1.2.0 branch since it contains the "Webdis version" of that release.

Also, please note that changing the polling behavior might have some consequences for Webdis that might be hard to test, related to conditions that aren't necessarily encountered during testing. Things like disconnections, timeouts, etc. It is possible that there are assumptions in the Webdis code base about which events will be triggered for these various situations, and that changing this behavior could have unexpected consequences. Typically what would be affected is whether callbacks are triggered for certain events, with what parameters, and in what order.

Finally, you mentioned this:

My intention is use new function from version v1.2.0.

Which function is that? A change was made for Webdis in May to expose a relatively new feature of Hiredis, namely the ability to control some keep-alive settings. If there are other new features that might be of interest to Webdis users, those could also go under the same "hiredis" key in the config file.

In any case, I'd certainly be interested to see your findings once you have an update.

[MichalPopovic]

Thank you for pointing to this information. I had not realised they used header Upgrading to > 1.2.0 as after version 1.2.0. I will try to use master branch for testing and let you know.