Skip to content

Running Synchronous Code as Async Using Lambdas (Code Smell) #162

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

Open
BoggerByte opened this issue Feb 21, 2025 · 6 comments
Open

Running Synchronous Code as Async Using Lambdas (Code Smell) #162

BoggerByte opened this issue Feb 21, 2025 · 6 comments
Labels
enhancement New feature or request help wanted Extra attention is needed

Comments

@BoggerByte
Copy link
Contributor

Enhancement description

The TodoistAPIAsync class currently wraps synchronous API calls using run_async(lambda: self._api.method(...)). This approach does not truly make the operations asynchronous and introduces inefficiencies, such as blocking the event loop.

To properly support async functionality, the project should consider using httpx, which provides both sync (httpx.Client) and async (httpx.AsyncClient) interfaces. This would allow for a cleaner implementation of async API methods without relying on wrapping sync calls inside lambda functions.

The problem it solves

  • Prevents event loop blocking: The current approach does not truly offload work asynchronously and could degrade performance in async environments.
  • Simplifies the implementation: Removing unnecessary lambda wrappers makes the codebase cleaner and easier to maintain.
  • Provides true async support: Using httpx enables real async HTTP requests without requiring additional workarounds like run_async.

Additional information

  • The httpx library is well-supported and designed for both sync and async HTTP requests.
  • This change would improve compatibility with modern async frameworks while maintaining backward compatibility for sync users.
@BoggerByte BoggerByte added the enhancement New feature or request label Feb 21, 2025
@goncalossilva goncalossilva added the help wanted Extra attention is needed label Mar 30, 2025
@goncalossilva
Copy link
Member

Thanks for the suggestion, @BoggerByte! This makes sense, and it seems like httpx is a great default choice. My only concern is our existing feature of passing in a Session, which lets SDK users customize a myriad of things, including customized error handling, back-off, or timeouts.

I'm not very familiar with httpx, so I don't know if there is a simple, seamless, idiomatic approach like this.

@BoggerByte
Copy link
Contributor Author

Thanks for the thoughtful reply!

You're right - allowing users to pass in a custom Session is important. The good news is that httpx supports similar flexibility via custom Client/AsyncClient instances, and even more extensibility through custom transports, which can handle advanced use cases like retries, error handling, or test mocking.

We could accept a user-provided httpx.AsyncClient, or default to one internally to preserve flexibility while enabling true async I/O without run_async workarounds.

Happy to help sketch out a migration plan or open a PR if this direction sounds good.

@goncalossilva
Copy link
Member

The good news is that httpx supports similar flexibility via custom Client/AsyncClient instances, and even more extensibility through custom transports, which can handle advanced use cases like retries, error handling, or test mocking.

How would you envision the API for it? Right now, TodoistAPI, takes an optional Session. Would it take an optional client? Or an optional transport?

@BoggerByte
Copy link
Contributor Author

I’d lean toward accepting an optional httpx.AsyncClient (or httpx.Client for sync use cases) since that maps most closely to the current Session parameter and gives users full control over things like headers, retries, timeouts, etc.

Internally, if no client is passed in, we can instantiate a default one with sensible settings.

As for Transport, it’s more of a power-user feature that is useful for advanced mocking, instrumentation, or custom retry logic. So we could optionally expose that later or document how to build a client with a custom transport and pass it in.

So maybe something like this in the constructor:

def __init__(self, ..., client: Optional[httpx.AsyncClient] = None):
    self._client = client or httpx.AsyncClient(...)

That keeps the API simple and idiomatic while staying very flexible.

Does that sound aligned with what you had in mind?

@goncalossilva
Copy link
Member

Thank you for expanding! I've brought this internally for debate and will circle back to you soon.

@goncalossilva
Copy link
Member

Thanks for the patience, @BoggerByte!

We're generally good with migrating to httpx, provided we:

  • Retain the flexibility we currently have, by allowing the client to be provided as you proposed above.
  • Update documentation and examples accordingly.
  • Find a suitable replacement for the responses library we use to mock requests, be it an equivalent for httpx, or an improved testing pattern.

If all these are met, we see no problems with transitioning to httpx! Contributions are welcomed. 😄

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request help wanted Extra attention is needed
Projects
None yet
Development

No branches or pull requests

2 participants