Various improvements#5
Conversation
the uv_pipe_connect call in UnixStreamClient::connect references variables that don't exist. there's a similar call in the "libuv>=1.46" preprocessor branch but uses different variables that are presumably the new updated ones so i just copied them over.
Non-CMake build system users can use this to more conveniently link uvco (including uvco-curl and uvco-pqxx if present) into their project
While the headers (in particular uvco/bounded_queue.h) may be considered private implementation details, they are #include'd in other headers so must also be installed.
Otherwise its argument list will only be destroyed after another task is added, which doesn't make a lot of sense.
There is no reason it shouldn't be allowed to issue multiple write calls at once, libuv supports this and will just execute the writes one after another, and uvco doesn't do anything to prevent this from working either. On the other hand, the write functions are cancellation-unsafe, because libuv keeps a reference to the buffer vector for the entire duration of the write, and since there's no way to cancel a write the buffers, the vector, and the callback must be kept alive until the write fully completes.
A coroutine can destroy itself by passing control to a custom awaiter. That way no "done tasks" queue is needed.
Constructing and holding a CancellationBlock object prevents the coroutine that constructed it, as well as the coroutine that created that coroutine and so on from being destructed. StreamBase::write now uses this mechanism to avoid use-after-free when the write is async and the promise is destroyed before it finishes. Other uvco functions might need to be evaluated to see if they would need CancellationBlock. This requires adding a reference counting mechanism to the coroutines, which might add some overhead but hopefully not too much. Since type-erased coroutine handles cannot be used to access the reference counter, every place that uses those had to be patched to use the new CoroutineHandle type, and awaiters patched to be templated for every concrete promise type (which CoroutineHandle uses to extract a pointer to the reference counter). Another new primitive: detach(Promise<void>) allows running a task in the background just like TaskSet, but unlike TaskSet the task keeps itself alive on its own independently, and cleans up when it's done.
|
Thank you for looking so deeply into uvco, and finding these issues! Some of the items you describe were (lazy) tradeoffs, but the partial write is indeed a dumb issue I hadn't encountered before, and apparently the existing test coverage didn't check for it either. Currently there are still some issues with the CI build, but I will review your changes soon. |
|
I am going to admit that I did not run any of the unit tests on my changes until now. The docs and the CMake output told me I need something called "gtest" to run the tests. However, I looked for a package called "gtest" in my package manager and there was nothing. I only now realized that the thing I were supposed to be looking for was Google Test, and it has an appropriately-named package The unit tests fail to compile, and one of the failures reveals that I forgot to adapt SelectSet to the new reference counting system, oops! I was relying on compile errors to find places to adjust but if it's in a template that never gets instantiated by uvco itself it'll just compile successfully if I only build uvco itself.. Should be a simple fix. The more concerning thing to me is that the clang++ build is erroring out for a different reason. I only tested with g++ and it seems to have turned out that I ended up relying on specific quirks of the compiler that are different in clang++. My goodness. What's gonna happen with MSVC? |
additionally, add constexpr to IoVec constructor, just in case...
|
Build should work now. I can't tell what's going on with Clang here, because I can't reproduce that compiler error with neither Clang 17.0.6 nor 18.1.8 (guix doesn't package 18.1.3 so i can't use that). Probably a Clang bug that has since been fixed (I did add some |
2 participants
I am trying to use uvco to build a web application, and while doing so found a few bugs and areas in uvco that could be improved. I gathered the changes together as they are
smallmostly small and there's a few of them. This merge request addresses the following:PKG_CONFIG_PATH.TaskSetimproperly discards the user-passed coroutine, causing the objects in its argument list to linger for longer than it should.uv_try_writereturns success if a partial write occurs, and uvco just gives up with the rest of the write and returns the number of bytes written in that case. If however, the write would completely block, it would wait foruv_writeand return 0. I made it consistently perform a full write and since the return value is bogus anyway i replaced it with void. I didn't actually test if the original code actually misbehaves in reality but it's pretty apparent just by reading the code and libuv docs that it's incorrect. I did test it after my changes though and it doesn't exhibit the issues i predicted when writing to a slow reader.There's also one big issue I found but have not addressed since I am not quite sure how exactly to do that (see #4)EDIT: this is now addressed by this PR