-
Notifications
You must be signed in to change notification settings - Fork 149
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Feature Request: mechanism to control the number of the threads in the cpprestsdk's thread pool before startup #298
Comments
Hi Yang, give this a try. Apply this patch no_init_thpool.patch.txt, and in your program, Tell me if you come across crash or any other kinds of issues. Thanks |
I will try that next week. |
@JinmingHu-MSFT Thanks. Yang |
@JinmingHu-MSFT |
@yxiang92128 Well, the result is not terribly surprising. There've been hang issues in the SDK for a very long time, and some of them are never really fixed. It's more likely to trigger with fewer threads. |
@JinmingHu-MSFT can you explain what other hang issues the C++ REST SDK might have so we can be aware when support issues arise? |
@yxiang92128 Well, not cpprest sdk, it's in our azure-storage sdk. At least as far as I know, when uploading a blob with max-execution-time set and parallelism > 1, there's small possibility that it will hang or crash. Next time you come across this hang issue, you can tell me the related information (which API, max-execution-time set or not, parallelism), maybe it'll be helpful to fix it. |
@JinmingHu-MSFT Would you think of any other workaround to inhibit this behavior so it would only start one thread on startup without causing it to hang? Thanks for any hint. Yang |
Hi, It's me again. I got some questions on this topic. Sorry for the long post in advance.
Use case: our app works with different object stores and this can be switched at runtime. For this purpose we use different SDKs, as one might expect. So, my point is - once we connect to an Azure storage account and the threads are started, they can't be stopped anymore? Is there a workaround for this?
If so, why so many threads? Or each thread can actually work with one TCP connection at a time (blocking/waiting for response) and connections are being served using round-robin/other algorithm?
Sorry, I'm not that familiar with the win32. I tried figuring out, but could you give me a hint about where/what to look at? I saw the Thanks, |
No, you cannot change the number of threads once the thread pool is initialized.
Not that I know of. Unless you run storage sdk in another process and communicate via socket or pipe or something. But i guess you won't like this way.
Yes, cpprestsdk is asynchronous.
This is pre-defined in cpprestsdk. Maybe (i'm not sure, i'm not developer of cpprest project) besides network requests, cpprest can also handle some cpu-intensive async tasks.
Correct, cpprest only depends on boost on Linux. On Windows, it uses a built-in thread pool.
That |
Is the thread pool stateless then? I mean - switching between Azure storage accounts should be safe although the thread pool is not recreated, correct? |
Yes, it is safe to switch accounts. |
I applied the patch and it works pretty good on Linux. But it crashes on MacOS. Looks like a timing issue, as it doesn't happen every single time, but often enough. I've attached a repro case + a MacOS crash report. Seems like an issue with Stack from the crash report:
Please advise. Thanks, |
I'm wrong. I ran the test app on Linux multiple times (with a bash loop) and it broke as well. Here's the Linux stack:
At least it's much more verbose that stack. |
@kiril-kirov I looked through the code again. I did find some issues in the implementation of http client reuse part. Because we're using a static |
I don't think there's anything you can do. although I can think of something we storage sdk can do to properly clean up stuff so the program can elegantly exit. But i haven't tried it yet. |
Yes, it's non-Windows only. Yes, it's on exit, as the stacks show. |
@kiril-kirov Add a function to clean this |
Just clearing the app? Or some graceful stop of those clients needs to be done? |
@kiril-kirov that's a map of shared_pointer, so I guess just clearing the map is fine. |
It looks promising at the moment. Here's the whole diff, if someone's interested:
So, you need to call:
|
@Jinming-Hu, did this feature ever make it to a branch that was released? I currently have a master version (Tue Feb 23 14:00:43 or
I understand you provided a patch above, but I really don't want to maintain that (nor have I tried it with that patch) in a version of azure-storage-cpp. |
@jade2k11598 This feature is not officially supported and it's not stable enough so we don't want to maintain it. |
@Jinming-Hu, it's really not practical for a utility to spawn a pool of 40 threads. My application just needs to upload files to Azure (just one of its many other features), but it shouldn't have to take 40 threads just to do that. This utility alone is spawning more threads, than the other features in my application. It should be left to the user to determine how many threads should be utilized for this utility (usually optimized for the number of hardware cores running on the machine). Granted there should be a minimum of how many threads are required, but 40 is excessive. Not having that ability to dictate how many threads are dedicated to this utility, is a major drawback in design. |
Currently it starts 40 threads in the pool and there is no way to configure that. Our application limits the number of threads due to resource constraints. And I am wondering if we can configure that when initialize_blob_client.
Thanks,
Yang
The text was updated successfully, but these errors were encountered: