Skip to content

fix(core): simplify action dispatch, use mutex locking, and avoid unnecessary clones#31157

Merged
FrozenPandaz merged 1 commit intomasterfrom
cammisuli/tui-misc-changes
May 9, 2025
Merged

fix(core): simplify action dispatch, use mutex locking, and avoid unnecessary clones#31157
FrozenPandaz merged 1 commit intomasterfrom
cammisuli/tui-misc-changes

Conversation

@Cammisuli
Copy link
Copy Markdown
Member

Current Behavior

  • There are many unwraps that we can avoid by using parking_lot::mutex
  • There is an unnecessary task::spawn to dispatch actions. The channel is non-blocking and this is wasted cycles
  • There are a bunch of clones that are happening with the PtyInstances

Expected Behavior

  • Uses parking_lot::Mutex and removes unwraps()
  • Dispatches actions directly without going into tasks
  • Uses tuple derefs to avoid clones, and uses 2 instead of 4

Related Issue(s)

Fixes #

@Cammisuli Cammisuli requested review from a team as code owners May 9, 2025 17:41
@Cammisuli Cammisuli requested a review from FrozenPandaz May 9, 2025 17:41
@vercel
Copy link
Copy Markdown

vercel Bot commented May 9, 2025

The latest updates on your projects. Learn more about Vercel for Git ↗︎

1 Skipped Deployment
Name Status Preview Updated (UTC)
nx-dev ⬜️ Ignored (Inspect) May 9, 2025 5:42pm

@nx-cloud
Copy link
Copy Markdown
Contributor

nx-cloud Bot commented May 9, 2025

View your CI Pipeline Execution ↗ for commit 3c1ccb5.

Command Status Duration Result
nx affected --targets=lint,test,build,e2e,e2e-c... ✅ Succeeded 5m 24s View ↗
nx run-many -t check-imports check-commit check... ✅ Succeeded 19s View ↗
nx-cloud record -- nx-cloud conformance:check ✅ Succeeded 2s View ↗
nx-cloud record -- nx format:check ✅ Succeeded 1s View ↗
nx-cloud record -- nx sync:check ✅ Succeeded 1s View ↗
nx documentation ✅ Succeeded <1s View ↗

☁️ Nx Cloud last updated this comment at 2025-05-09 19:06:24 UTC

});
if let Some(tx) = &self.action_tx {
tx.send(action).unwrap_or_else(|e| {
debug!("Failed to dispatch action: {}", e);
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Food for thought, should this be error!()?

@FrozenPandaz FrozenPandaz merged commit f5cc2d5 into master May 9, 2025
6 checks passed
@FrozenPandaz FrozenPandaz deleted the cammisuli/tui-misc-changes branch May 9, 2025 19:40
@github-actions
Copy link
Copy Markdown
Contributor

This pull request has already been merged/closed. If you experience issues related to these changes, please open a new issue referencing this pull request.

@github-actions github-actions Bot locked as resolved and limited conversation to collaborators May 15, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants