Redis: Review bigger Windows-specific code blocks

[tporadowski]

Review bigger Windows-specific code blocks (#ifdef _WIN32) copied from win-3.2.100 as they may not be equivalent to newer 4.0.2 code any more

[israellot]

@tporadowski , I've reviewed those sections, and at the same time ported antirez commits up to 4.0.10 release
Please check https://github.com/israellot/redis/tree/4.0.10

I've ported only redis-server changes so far, didn't touch client, benchmark, etc.

[israellot]

Update : ported the 4.0 branch up until the last commit.

[israellot]

Did you get a chance to review this @tporadowski ?

[tporadowski]

@israellot I didn't forget about it, but I wish I had some time to do this... Meanwhile, did you try to set up your testing environment (I'm using Cygwin) and run the unit tests on your fork?

[israellot]

It seems this thread got lost on my notifications.
I didn't run the unit tests. Are you using the tcl project? I was under the impression that wouldn't even work on windows.

[israellot]

Meanwhile, I've ported the latest commits from Antirez's upstream. Ported one by one and kept the same description to make it easier to understand.

[tporadowski]

Yes, I'm using Cygwin and its Tcl package as "tclsh" is required. Once you have the binaries built you need to make links in main "src" folder (with "mklink" command) as test runner will expect them to be there (e.g. "src/redis-server" - please note the lack of ".exe" extension). You might also need to use a limited number of clients (there is a command-line option to tests/tcl_helper) as this happens to not always work reliably with Redis for Windows (MS OpenTech mentioned this in their readme), which in turn could be related to some timeout limits set up in the unit tests.

[israellot]

Thanks for your guidance.
Most tests passed but :

• unit/quit ( fail trying to read output stream, maybe not related to redis? )
• Turning off AOF kills the background writing child if any in tests/unit/aofrw.tcl
• HyperLoglog unit test causes redis to crash with exception on networking.c

=== REDIS BUG REPORT START: Cut & paste starting from here ===
Redis version: 4.0.13
[31568] 14 Mar 18:33:40.713 # --- EXCEPTION_ACCESS_VIOLATION
[31568] 14 Mar 18:33:40.713 # --- STACK TRACE
redis-server.exe!StackTraceInfo(C:\Repos\redis\src\Win32_Interop\Win32_StackTrace.cpp:153)(0x140168860, 0x0014FF60, 0x0014A8E0, 0x0014A8E0)
redis-server.exe!UnhandledExceptiontHandler(C:\Repos\redis\src\Win32_Interop\Win32_StackTrace.cpp:186)(0x14002BAB0, 0x14002BAB0, 0x0014A8E0, 0x00000000)
KERNELBASE.dll!UnhandledExceptionFilter(C:\Repos\redis\src\Win32_Interop\Win32_StackTrace.cpp:186)(0x00000000, 0x7FFD15CB33C4, 0x00000000, 0x00000000)
ntdll.dll!memset(C:\Repos\redis\src\Win32_Interop\Win32_StackTrace.cpp:186)(0x0014B080, 0x0014AF00, 0x0014B570, 0x0014AEA8)
ntdll.dll!_C_specific_handler(C:\Repos\redis\src\Win32_Interop\Win32_StackTrace.cpp:186)(0x00000000, 0x0014AE90, 0x00000000, 0x140000000)
ntdll.dll!_chkstk(C:\Repos\redis\src\Win32_Interop\Win32_StackTrace.cpp:186)(0x0014AE90, 0x00000000, 0x7FFD1481BD08, 0x7FFD14770000)
ntdll.dll!RtlWalkFrameChain(C:\Repos\redis\src\Win32_Interop\Win32_StackTrace.cpp:186)(0x00000000, 0x00000000, 0x7FA500000000, 0x00000000)
ntdll.dll!KiUserExceptionDispatcher(C:\Repos\redis\src\Win32_Interop\Win32_StackTrace.cpp:186)(0x00000000, 0x00989680, 0x0096D59E, 0x1400AF6C3)
redis-server.exe!prepareClientToWrite(C:\Repos\redis\src\networking.c:189)(0x0014B8A0, 0x00000009, 0x00000000, 0x00000000)
redis-server.exe!addReply(C:\Repos\redis\src\networking.c:339)(0x411DC13000000000, 0x0096D59E, 0x00989680, 0x00000000)
redis-server.exe!pfselftestCommand(C:\Repos\redis\src\hyperloglog.c:1471)(0x1D4DA8C0FB9E50A, 0x5841152A5F081, 0x00000000, 0x7FA55ACF7000)
redis-server.exe!call(C:\Repos\redis\src\server.c:2321)(0x7FA55ACF7000, 0x140083B10, 0x7FA55ACF7000, 0x1400F4DA0)
redis-server.exe!processCommand(C:\Repos\redis\src\server.c:2610)(0x00000001, 0x7FA55ACF7000, 0x7FA55ACF7000, 0x140083B10)
redis-server.exe!processInputBuffer(C:\Repos\redis\src\networking.c:1482)(0x7FA55ACF7000, 0x00000015, 0x00000000, 0x00000000)
redis-server.exe!readQueryFromClient(C:\Repos\redis\src\networking.c:1576)(0x7FA55AC6F100, 0x00000000, 0x00000001, 0x7FA55AC0D1A0)
redis-server.exe!aeMain(C:\Repos\redis\src\ae.c:531)(0x004247C0, 0x58411529C9B93, 0x1401A81A0, 0x7FA55ACF8000)
redis-server.exe!redis_main(C:\Repos\redis\src\server.c:4065)(0x00425F90, 0x00425F90, 0x00000000, 0x00000002)
redis-server.exe!main(C:\Repos\redis\src\Win32_Interop\Win32_QFork.cpp:1285)(0x00000000, 0x00000000, 0x00425F90, 0x00000000)
redis-server.exe!__scrt_common_main_seh(d:\agent_work\3\s\src\vctools\crt\vcstartup\src\startup\exe_common.inl:288)(0x00000000, 0x00000000, 0x00000000, 0x00000000)
KERNEL32.DLL!BaseThreadInitThunk(d:\agent_work\3\s\src\vctools\crt\vcstartup\src\startup\exe_common.inl:288)(0x00000000, 0x00000000, 0x00000000, 0x00000000)
ntdll.dll!RtlUserThreadStart(d:\agent_work\3\s\src\vctools\crt\vcstartup\src\startup\exe_common.inl:288)(0x00000000, 0x00000000, 0x00000000, 0x00000000)
ntdll.dll!RtlUserThreadStart(d:\agent_work\3\s\src\vctools\crt\vcstartup\src\startup\exe_common.inl:288)(0x00000000, 0x00000000, 0x00000000, 0x00000000)

[tporadowski]

@israellot I've tried running tests on your 4.0.10 branch, but it failed pretty fast on "unit/type/string" where I had to solve some issue with type casting (usage of ssize_t started to cause problems). Anyway, so far I made it to test "30/47 integration/psync2", but it crashes the test suite. When I get this solved then I can see if it also fails for me in the same places as for you - as I have a feeling it's either pretty random or very build-environment dependent...

[israellot]

Thanks for the feedback @tporadowski
I'm not sure why mine hasn't failed on the string test. Let me know that goes.
Hopefully, we can catch up on branch 4.X

[tporadowski]

At first I thought it was related to VS2019 and latest Windows SDK I upgraded to, but it also failed when built with VS2017... I will check my branch if that happens there now as this might just be some issue in my environment.

[tporadowski]

Some types of parameters changed and i.e. src/sds.c/sdsIncrLen() now accepts ssize_t incr while previously it was int incr, but there is still deps/hiredis/sds.c/sdsIncrLen() that accepts int, so maybe compiler gets confused or something. I will try to make it stable and proceed with tests.

[tporadowski]

@israellot you have an inconsistency between src/sds.h and src/sds.c related to sdsIncrLen() - the former uses "int", while the latter "ssize_t", which caused that strange failures in tests. Yet, I wonder why it popped up only for me. Anyway, after correcting this in src/sds.h I can move forward.

[israellot]

That's interesting, a problem would only appear if the length would actually surpass the 32bit barrier of an int I suppose. Anyway, good catch.

[tporadowski]

It was quite opposite - when a buffer longer than 32KB (I guess) is received sdsIncrLen function gets called with -2 as increment in src/networking.c line 1369 to get rid of CRLF, but due to that type mismatch it was converted for some reason to an unsigned int with a huge value (probably resulting from an overflow). That was causing this assert to fail immediately as there was not enough space there to accommodate such a long string (well, long due to that type overflow).

[israellot]

Meanwhile, I've replicated the latest commits from Antirez's branch, which include some critical bug fixes according to him.

[tporadowski]

I think I'll send you a patch as otherwise I don't know what happens if I clone your repo (wh8ch is a clone of mine ;) )

[israellot]

@tporadowski if your fixes aren't many, the patch will do just fine.

[tporadowski]

@israellot I've managed to get latest code from your fork (4.0.14 now, I believe) to pass the tests, but one fails randomly (unit/limits) and once I find the reason I will send you my changes.

[tporadowski]

Or I can make a branch for you to open a PR, then I'll add those...

[israellot]

Did you ever get to figure out the last problematic test?

[tporadowski]

@israellot not really, it was random. Please create a pull request to https://github.com/tporadowski/redis/tree/4.0.10 and I'll add my changes there after merge and we'll see how it goes :)

[israellot]

Hi @tporadowski , just checking to see if you were able to add your changes to the branch. Thanks

[israellot]

If you want, I can try to merge them. Just point me the directions you have in mind.

[tporadowski]

@israellot thanks for friendly reminders ;), I will try to commit those changes this weekend

[tporadowski]

I am working on it since last weekend, but after moving to a new laptop somehow even basic tests started to fail, although things look fine (when testing "by hand") - I'm investigating those Tcl-based test scripts now.

[israellot]

It really got me a while to have tcp working properly as well.

[israellot]

Is there anything I can do to help?

[israellot]

Hate to ask, but can you please commit and let me try to figure out the tests?
@tporadowski

[tporadowski]

I will have to try it again from my old laptop as on the new one all those branches got a little messy, not to mention those weird test results. I will try to do this this weekend.

[israellot]

Thank you @tporadowski , but you can also send me the patches and I can try to work out the tests myself.

[tporadowski]

@israellot please give it a try - I copied over binaries to my new laptop and it failed pretty early on unit tests, but it worked fine on my old laptop - this is very, very weird...
This patch is against your fork, but probably on some older commit (like 4.0.11 or around)
redis-israellot.patch.txt

[israellot]

Success
I'm pushing the updates to the branch.