-
Notifications
You must be signed in to change notification settings - Fork 741
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
Python: Make AzureOperationPoller thread a daemon thread #1379
Conversation
Can one of the admins verify this patch? |
Hi @tjprescott, I'm your friendly neighborhood Azure Pull Request Bot (You can call me AZPRBOT). Thanks for your contribution!
TTYL, AZPRBOT; |
Is http://bugs.python.org/issue1856 of concern with this change (for Python [2.x)?] |
@azuresdkci test this please! |
@annatisch @lmazuel Is this something we want to add to the python client runtime? |
Hum.... I see your bug report @tjprescott and it makes senses. Daemon thread cannot prevent the main executable to quit. I think it's relevant. Good point. |
And I'm thinking it's one of the reason I got the test UI in PTVS unable to launch my tests again when I cancel a unittest that use LRO (need to quit PTVS and relaunch it). @zooba what do you think? |
@lmazuel I tested the fix's ability to break out of our CLI in both Python 2.7 and 3.5 virtual environments and it worked as expected. On Linux you need only press Ctrl+C once and it breaks to command line. On CMD you press it once and you'll get the "Terminate batch operation Y/N" prompt which is much better than before where you had to press it multiple times and still incur a lengthy polling delay before you would get that prompt. This is the simplest fix but I don't know that there wouldn't be applications that specifically would NOT want daemon threads. If that is the case it might be better to make it an optional parameter when the LRO is created. |
@annatisch I think it's best to accept the PR, it should be the default. However, we might want to add a configuration item in the azure_configuration object for this. In this case, we create another issue (low priority) like "Add daemon thread LRO parameter to azure_configuration". Thoughts? |
@lmazuel Sounds good :) |
Created the issue for conf: #1416 |
Merged |
See item #1378. This PR doesn't make the use of daemon threads configurable, but that is an option too. This is the simplest fix.