fix(yfinance): prevent indefinite hang in etf.info#7319
fix(yfinance): prevent indefinite hang in etf.info#7319biplavbarua wants to merge 5 commits intoOpenBB-finance:developfrom
Conversation
Wraps the Ticker.get_info() call with asyncio.wait_for(timeout=30) to prevent the application from hanging if yfinance is unresponsive.
|
How can I recreate the hanging? |
|
@deeleeramone To reproduce the hanging, you can simulate a blocking call in The issue is that Here is a standalone script to reproduce the behavior: import asyncio
import time
# Simulate the blocking call (yfinance.Ticker.get_info)
def mock_get_info_blocking():
# Simulate a hang (e.g. socket timeout or long response)
# This sleep is longer than the 30s timeout we set
time.sleep(60)
return {}
async def main():
print('Testing functionality...')
# attempts to run blocking code with a timeout
try:
await asyncio.wait_for(
asyncio.to_thread(lambda: mock_get_info_blocking()),
timeout=5
)
except asyncio.TimeoutError:
print('SUCCESS: Timeout caught, application recovered.')
if __name__ == '__main__':
asyncio.run(main())Without the |
That's exactly what I'm asking for. If it was hanging, it would be a problem for ALL the yfinance endpoints. What is the non-simulated way to reproduce? |
|
@deeleeramone You are correct that this issue potentially affects all yfinance endpoints that rely on blocking calls. It is difficult to deterministically reproduce a 'real' network hang against the live Yahoo Finance API without using network shaping tools. However, the mechanism of failure is clear: yfinance makes synchronous requests, and if the underlying socket hangs, the thread spawned by asyncio.to_thread gets stuck. To address this comprehensively: I have identified all locations where Ticker(...).get_info() or similar blocking methods are used. |
|
Self-review looks good. Consolidating the |
|
What I'm really interested in here, @biplavbarua, is how did you get it hang in the first place? What is the context for how you are getting it to hang? Are you spamming it like crazy, or what else is happening? If this was a problem, Ticker.info wouldn't be the only place it exists. |
|
@deeleeramone Thanks for the review!
The indefinite hang occurs because
You are absolutely right. I've updated the PR to apply the timeout wrapper (
I also fixed the linting errors (assignments of unused specific imports) that caused the CI failure. I ran the local tests for the yfinance provider and they are passing. |
As I'm saying, Ticker.info isn't the only thing that makes network requests. If this was actually a problem, it would be ALL the endpoints. |
Do you not have error handling? Seeing as you are spawning threads, how do we know that your implentation and handling is not creating race conditions or other scenarios that are actually causing the thread to "lock"? yFinance library itself already applies a 30 second timeout, so this wouldn't really play out in reality like you are describing. |
|
Thanks for the review. You are correct that The proposed |
You are describing problems with your own implementation though. Additionally, you have no evidence to support that this would actually do anything for your locked threads. I would look at your internal code and properly handle thread safety there instead of chasing something that isn't the root cause of your problem. |
yFinance itself is a Singleton, so the only way you create a "new instance" is to use a separate interpreter entirely with no shared resource at all. |
|
You are right that the root cause was likely shared state (specifically the session/connection pool) causing deadlocks in a threaded environment, and that a simple timeout was just a band-aid. To address this properly, I've updated the implementation to enforce session isolation. We now instantiate a fresh I verified this locally with a script spawning concurrent |
|
I think you are misunderstanding me. The thread locking you are experiencing is the result of your production server code implementation, and I am suggesting that you fix it there instead of chasing something that isn't the actual root of the problem. |
|
@deeleeramone Thanks for the insight regarding the Singleton usage and its impact on threading in production. I appreciate you pointing that out. While I agree that the root cause likely stems from the concurrency model in my production environment, I still believe this change has value as a defensive programming measure. Currently, Would you be open to this change if I frame it strictly as a "safety mechanism" (e.g., exposing an optional If this is still out of scope for OpenBB, let me know and I'll go ahead and close the PR to focus on my server-side implementation. |
This PR fixes a bug where getting ETF info would hang indefinitely if yfinance was unresponsive.
Changes:
Verification: