multi: add curl_multi_wakeup()

[ngg]

This commit adds curl_multi_wakeup() which was previously in the TODO
list under the curl_multi_unblock name.

On some platforms and with some configurations this feature might not be
available or can fail, in these cases a new error code
(CURLM_WAKEUP_FAILURE) is returned from curl_multi_wakeup()

Fixes #4418

This PR is a work-in-progress, I'm submitting it to receive feedback about
the design choices and the implementation.
I've tested it using our application on Windows, Linux, macOS and Android
platforms, and it seems to work well.
I'm not sure how to add a test case for this in the curl test suite, do you have
an idea? Or probably some example should be added as well.

[bagder]

Why do we need CURLMOPT_ENABLE_UNBLOCK for this? Why not just enable this unconditionally ?

[ngg]

If it is enabled then every multi uses two additional fds and the initialization time will be a bit slower. If someone never uses unblock, these are really unnecessary.

Secondly, because Curl_socketpair can fail (mostly on Windows), we'd like to know before calling curl_multi_poll() whether this unblock feature will work, so we can adjust the timeout based on that.

[ngg]

My second reason in the previous reply is really weak: we can simply call curl_multi_unblock() and see if it succeeda before the first poll to adjuat timeouts.

[ngg]

On second thought, this perf penalty doesn't seem that huge considering every DNS lookup will also create a socketpair when using the threaded resolver. I'll update this PR to have it enabled automatically.

Also it'd be easier to not include a new error code, but I couldn't find any that looked good. Should I reuse one of the existing codes and if so then which one?

[ngg]

I've removed the setopt option, now it is enabled if USE_SOCKETPAIR is defined and USE_BLOCKING_SOCKETS is not (blocking write socket can cause a deadlock if unblock is called from the same thread that will call poll).

Idea: After this is merged probably the threaded resolver can be refactored to call unblock after it is done instead of creating a new socketpair every time.

[bagder]

I'm not sure how to add a test case for this in the curl test suite, do you have an idea?

Two ideas:

1. we can easily add test a case or two that use unblock when nothing is blocking, and make sure the behavior is correct for that case.
2. we can use pthread (inspiration from test 1541) to fire up a test program that waits in one thread and we unblock it in a separate one. We just need to make some clever ways to provide the feedback when it works that doesn't depend on very exact timing.

Or probably some example should be added as well.

Yes and once we have the docs and tests worked out, that should be really easy!

[ngg]

Thanks for the quick feedback! I'll try to address the implementation and docs issues today, and I'll try to add tests after that.

[ngg]

I have another question:
On platforms where unblock is available and initialization succeeds, inside Curl_multi_wait there will always be at least that fd to poll for. This means that basically curl_multi_wait() also becomes like curl_multi_poll() by always waiting for something. As that is technically almost the same breaking change for curl_multi_wait() for current API users as it would be to simply replace it with curl_multi_poll(). Probably we should enable polling this socketpair only inside curl_multi_poll() where it doesn't cause different behavior if the feature is not used. What do you think?

[bagder]

Ah, good thinking - I hadn't considered that subtlety!

Yes, I would be fine with making it only work for curl_multi_poll() since it helps us not break the existing behavior of curl_multi_wait(). The poll version is the one we rather direct new users to anyway.

[ngg]

I've updated the implementation and docs, and also added the first test which tests unblocks and poll from a single thread without any easy handles attached. I'll add more tests (including a multi-threaded one) and an example later.
I have some questions:

• Test case numbering is not clear to me, is 1564 a good number for this new test? (other curl_multi_* tests were in 15xx range)
• This unblock feature is really about timing, which will make the tests timing dependent a bit. If I increase the timeouts then the tests will become slower but less flaky, how much time is acceptable for these new tests to take?

[pps83]

Might improve the description, as unblocks a "hanging" curl_multi_poll call. - sounds like a call that hung (eg deadlocked).

[ngg]

The "unblock" word itself can sound a bit horrifying I chose this because it was already in the TODO sectiom, personally I'd prefer to call this "wakeup". The description then could be something like " wakes up a sleeping curl_multi_poll call". Ideas? Or if we leave it named unblock, what description should we use?

[bagder]

Naming is hard. I don't have any problems with renaming it *_wakeup if you think that's better.

Is there any previous art somewhere for a similar API that we could copy the name from?

[pps83]

I was mainly referring to "hanging" in the description. not the _unblock part. Description perhaps should make it clear that the api unblocks/wake up another thread that's blocked on/waiting for curl_multi_poll calls

[ngg]

Now it is called curl_multi_wakeup and the short description says wakes up a sleeping curl_multi_poll call, I hope this is better.

[pps83]

• wouldn't that be confusing to have AF_UNIX even on windows? (I know that it's ignored inside Curl_socketpair)
• why sock_stream? udp should to be lighter than tcp.

[bagder]

wouldn't that be confusing to have AF_UNIX even on windows? (I know that it's ignored inside Curl_socketpair)

Won't it be equally confusing to setup an #ifdef for the value when it isn't even used on that platform?

why sock_stream? udp should to be lighter than tcp

Well, we want the pipe to be a reliable stream. I'm not sure what to expect from this if we use datagrams instead... besides, does it really matter (can the "overhead" even be measured?) for this simple use case?

[ngg]

I chose these parameters because this is how it is already used in the threaded resolver. These can be later improved in both places in a separate PR. Based on https://stackoverflow.com/questions/13953912/difference-between-unix-domain-stream-and-datagram-sockets I think SOCK_STREAM is better here because if we always send 1-byte messages then we would only be able to read a single byte at a time with SOCK_DGRAM (Note: I didn't test if this is really the case)

[bagder]

Test case numbering is not clear to me, is 1564 a good number for this new test? (other curl_multi_* tests were in 15xx range)

The number just have to be unique, the grouping or ranges are only for humans so you can go however you think is fine. 1564 looks perfect to me!

This unblock feature is really about timing, which will make the tests timing dependent a bit. If I increase the timeouts then the tests will become slower but less flaky, how much time is acceptable for these new tests to take?

If we can't provide any other hints to help figure that out in the test, I guess we need to test with something and see what works...

[ngg]

The latest CI run shows it seems to work everywhere except on Windows when compiled with MSYS.

In that case, the tests can fail at libtest/lib1564.c:120 curl_multi_poll returned too early which tests that even if there are a huge amount of unblock calls, those all wakes up the first poll only (poll should read every byte from the socketpair).
I have no idea why this happens, I think there is some difference between normal Windows winsock and MSYS winsock calls (probably related to (WSA)EINTR handling or something similar).

I have currently no access to a Windows machine with MSYS, so I'll add some debug logging of error codes inside curl and let appveyor build that.

[ngg]

Strange, based on these debug messages it seems that writing to these socket is much more asynchronous than it is on any other platform, meaning that if we write to this socket then later on that same thread it's possible the data is not even ready to be read.
The good thing is that it means it's a problem with the test and not with the implementation.
For a test workaround, as it only seems to happen only on Windows and only when we write a lot to the socket, I think it might be fixed by adding a single sleep (only on Windows) after the large cycle that calls unblock and before the poll call. I'm trying that now.

[ngg]

I think I'm ready with this PR, please review. In the last CI run all failures seemed to be unrelated to this change. I've started another one just to make sure.

Notable changes since initial version:

• fixed initial issues that came up on first review
• the socketpair is created during curl_multi_init (and not with a setopt)
• function has been renamed to curl_multi_wakeup
• wakeup only affects curl_multi_poll (and not curl_multi_wait)
• calling wakeup a lot of times during (or before) a single poll call should only wake up that call and not later calls. This however is not guaranteed and there are cases (on Windows where the socketpair is just too asynchronous and we cannot reliably read out all data from it)
• docs has been updated to reflect these changes
• one test (1564) has been added that tests call wakeup and poll from a single thread to see how they interact
• a multi-threaded test (1565) has been added that tests a more realistic scenario