Skip to content
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

removing runtimes #345

Merged
merged 5 commits into from
Mar 19, 2024
Merged

removing runtimes #345

merged 5 commits into from
Mar 19, 2024

Conversation

Kayanski
Copy link
Contributor

@Kayanski Kayanski commented Mar 14, 2024

This PR aims at using an existing runtime by default when no runtime is specified in the DaemonBuilder.
This allows removing runtime creation for users.
Removed custom handles in the whole library

Chat-GPT says :

Using lazy_static with Tokio runtimes can be a convenient approach in certain scenarios, but it's important to understand the implications and potential drawbacks.

lazy_static is a Rust crate that allows you to create lazy-initialized static variables. This means that the variable is initialized the first time it's accessed and then reused for subsequent accesses. This can be useful for initializing expensive resources once and sharing them across multiple parts of your codebase.

However, when it comes to Tokio runtimes, there are a few considerations:

Thread Safety: Tokio runtimes are not thread-safe by default. They are designed to be used within a single-threaded context or within the context of a Tokio executor. If you use lazy_static to create a global Tokio runtime, you need to ensure that it's accessed in a thread-safe manner, especially if your application is multi-threaded.

Resource Management: Tokio runtimes manage resources such as threads and event loops. Creating a global Tokio runtime with lazy_static means that these resources will be initialized and held for the lifetime of your application, which may not be ideal for all use cases. It could lead to resource wastage if the runtime is initialized but not actively used.

Flexibility: Using lazy_static to create a global Tokio runtime might limit the flexibility of your codebase. It can make it harder to reason about where and how the runtime is being used, and it may not accommodate scenarios where you need multiple Tokio runtimes with different configurations.

Testing: Global state can make unit testing more challenging. If your code relies on a global Tokio runtime created with lazy_static, it may be difficult to mock or substitute the runtime for testing purposes.

Given these considerations, it's generally recommended to carefully evaluate whether using lazy_static for Tokio runtimes aligns with the requirements and design of your application. In many cases, it might be preferable to explicitly manage the creation and lifecycle of Tokio runtimes within the scope of your application or library, rather than relying on global state.

If you do decide to use lazy_static with Tokio runtimes, make sure to thoroughly test your code and consider the potential trade-offs in terms of thread safety, resource management, and code maintainability. Additionally, document the usage and constraints of the global Tokio runtime to make it clear for other developers working on the codebase.

Copy link

cloudflare-workers-and-pages bot commented Mar 14, 2024

Deploying with  Cloudflare Pages  Cloudflare Pages

Latest commit: 87f9a16
Status: ✅  Deploy successful!
Preview URL: https://5df0f6da.cw-orchestrator.pages.dev
Branch Preview URL: https://solving-runtime.cw-orchestrator.pages.dev

View logs

Copy link

codecov bot commented Mar 14, 2024

Codecov Report

Attention: Patch coverage is 55.55556% with 4 lines in your changes are missing coverage. Please review.

Project coverage is 66.6%. Comparing base (40a8a0f) to head (87f9a16).
Report is 1 commits behind head on main.

Additional details and impacted files
Files Coverage Δ
cw-orch-daemon/src/sync/builder.rs 60.9% <100.0%> (-9.8%) ⬇️
cw-orch-daemon/src/sync/core.rs 69.8% <ø> (+0.6%) ⬆️
cw-orch-daemon/src/live_mock.rs 51.8% <50.0%> (-0.3%) ⬇️

Copy link
Contributor

@Buckram123 Buckram123 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔥

Comment on lines 102 to 103
let rt_handle = self.handle.clone().unwrap_or(RUNTIME.handle().clone());

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't it be unwrap_or_else so we don't spawn runtime, when only custom used?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, the runtime in spawned anyway in the lazy static. But yeah we don't need to get the handle, will add

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, I thought lazy_static code only executed when used, sorry

@Kayanski Kayanski merged commit 424a611 into main Mar 19, 2024
19 of 20 checks passed
@Kayanski Kayanski deleted the solving-runtime branch March 19, 2024 15:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants