feat: async rename and create file #1870

yioneko · 2022-12-31T02:06:33Z

Split of #1863.

New experimental options:

  experimental = {
    async = {
      create_file = false,
      rename_file = false,
    },
  },

I select these two operations because their async version is nearly 1to1 copy of the original implementation, and the risk of breaking is bit lower. Also, I think we could turn on rename_file by default, and let users choose to enable create_file either if they feel well about the new async behavior.

gegoune

Good work!

lua/nvim-tree/actions/fs/create-file.lua

lua/nvim-tree/events.lua

alex-courtis · 2023-01-02T23:24:43Z

Thanks for the updates @yioneko

This PR deserves significant review time which I can allocate this weeken.

yioneko · 2023-01-05T02:04:24Z

Just post the discussion about async lib in core: neovim/neovim#19624.

My current implementation is quite minimal so it might be fairly convenient to migrate to it once standardized. Also, it's reasonable to suspend this pr unless the async lib is implemented in core, and we could use it directly. Open to any suggestions.

alex-courtis · 2023-01-07T02:51:18Z

Just post the discussion about async lib in core: neovim/neovim#19624.

My current implementation is quite minimal so it might be fairly convenient to migrate to it once standardized. Also, it's reasonable to suspend this pr unless the async lib is implemented in core, and we could use it directly. Open to any suggestions.

It sounds a core implementation is quite far away.

I'm quite happy to have the implementation in nvim-tree and migrate later, which should be trivial.

alex-courtis

I'm happy to run with this; experimental features are... experimental.

Please:

Revert :help formatting changes
Remove API
Revert unnecessary events changes
Revert bulk move
Revert open file changes
Normalise create-file - DRY
~~Force and assume filesystem_watchers.enable~~
Use "standard" windows feature flag detection: feat: async rename and create file #1870 (comment)
More useful error messages: feat: async rename and create file #1870 (comment)

doc/nvim-tree-lua.txt

alex-courtis · 2022-12-31T02:39:07Z

doc/nvim-tree-lua.txt

@@ -1134,6 +1140,21 @@ Configuration for diagnostic logging.
        |nvim-tree.filesystem_watchers| processing, verbose.
          Type: `boolean`, Default: `false`

+*nvim-tree.experimental*


TODO @alex-courtis expand on these and think about how to present them in the readme/wiki etc.

doc/nvim-tree-lua.txt

lua/nvim-tree/actions/node/open-file.lua

lua/nvim-tree/actions/fs/rename-file.lua

lua/nvim-tree/async.lua

lua/nvim-tree/events.lua

lua/nvim-tree/actions/fs/create-file.lua

alex-courtis · 2023-01-14T04:56:07Z

@yioneko I just discovered that review comments do not get posted until the review is complete.

Please accept my apologies; I assumed you were able to see my comments from ~weeks ago.

yioneko · 2023-01-14T07:29:45Z

@alex-courtis Uhh, that's fine, I'm also busy during this period. Thanks for the detailed comments, I'll work on this next week.

alex-courtis · 2023-01-14T23:06:20Z

@alex-courtis Uhh, that's fine, I'm also busy during this period. Thanks for the detailed comments, I'll work on this next week.

No worries. This is valuable functionality, but there is no rush.

yioneko · 2023-01-20T03:38:17Z

lua/nvim-tree/async.lua

+
+M.Interrupter = Interrupter
+
+---This is useful for cancelling execution async function


One more pending question to make this ready for merge: should we implement cancellation of these operations? This is impossible in sync version but achievable now.

YAGNI.

We can add cancellation when we have a use case.

alex-courtis

This is looking good; we will get there.

Please:

remove unused codepaths
resolve my naive questions around the need to schedule
don't use async prompts

lua/nvim-tree/actions/fs/rename-file.lua

alex-courtis · 2023-01-22T01:17:19Z

lua/nvim-tree/async.lua

+
+M.Interrupter = Interrupter
+
+---This is useful for cancelling execution async function


YAGNI.

We can add cancellation when we have a use case.

lua/nvim-tree/async.lua

alex-courtis · 2023-01-22T01:57:38Z

lua/nvim-tree/async.lua

+  end)
+end
+
+---Asynchronous `vim.schedule`


Why do we need to do this? Please add comments here and at call sites as to why.

As documented in :h lua-loop-callbacks, we cannot call some vim.api.* and all vim.fn.* in luv callbacks. So every time before these calls, we should manually call vim.schedule (async.schedule here to avoid callback hells). It's my fault to assume that we already get this; I'll add necessary comments about it.

I see. So this schedule is essentially a thread join.

I do not understand the timing; surely it should be called immediately after the .call or during the callback. It's called once at the start of create_file and once at the end.

Certainly we could do that for every callback, but usually this is only needed when the callback contains vim apis (or we are uncertain about this, such as user provided callback). In some cases we may not want to use vim.schedule to delay the execution of callback like a sequence of consecutive luv calls and always delaying may have potential negative impact on performance.

I see. In that case, please move the calls to schedule immediately after the calls that require this join. It should be clear exactly when and where it is required.

lua/nvim-tree/async.lua

alex-courtis · 2023-01-22T02:12:11Z

lua/nvim-tree/actions/fs/create-file.lua

+        local prompt_select = "Overwrite " .. new_file_path .. " ?"
+        local prompt_input = prompt_select .. " y/n: "
+        if M.enable_async then
+          local item_short = async.call(lib.prompt, prompt_input, prompt_select, { "y", "n" }, { "Yes", "No" })


Do we need to make the prompt async? Only create_and_notify etc. needs to be async.

Yes, this is mandatory in async version. In fact, lib.prompt itself is already async because of usage of callback, the only difference is that the async context of lib.prompt is not controlled. If we use callback instead of wrapping with async.call, create_and_notify may not be called in the same async context as where async.exec begins and the immediate following async.call in create_and_notify will throw error.

You can test it with overriding vim.ui.input with https://github.com/stevearc/dressing.nvim or even a simple delayer:

local original = vim.ui.input vim.ui.input = function(opts, on_confirm) original(opts, function(input) vim.defer_fn(function() on_confirm(input) end, 0) end) end

In principle, all callback-style functions should be wrapped to make the async context consistent.

Unfortunately I still don't understand. Yes, the vim.ui.select calls themselves are asynchronous, however the callbacks appear to be executed on the main thread / context.

function M.prompt(prompt_input, prompt_select, items_short, items_long, callback) ... if M.select_prompts then vim.ui.select(items_short, { prompt = prompt_select, format_item = format_item }, function(item_short) log.line("dev", "prompt select in_fast_event=%s is_thread=%s", vim.in_fast_event(), vim.is_thread()) callback(item_short) end) else vim.ui.input({ prompt = prompt_input }, function(item_short) log.line("dev", "prompt input in_fast_event=%s is_thread=%s", vim.in_fast_event(), vim.is_thread()) callback(item_short) end) end end

[2023-01-23 10:29:44] [dev] prompt input in_fast_event=false is_thread=false

Yes, because it might be called in main thread (not in async thread), we need to use async.call to transfer it to async thread, otherwise create_and_notify will throw error because async.call uses coroutine.yield under the hood.

async.exec is not required to be called from main thread. It's possible to create new coroutine in another coroutine. The coroutine (or thread, here) is just stackful statemachine and could be created anywhere.

Would you mind I make a simple guidance for understanding of current basic async implementation? I've seen that there are plenty of misunderstandings between us and possibly with future contributors.

Yes please. A detailed explaination would be gratefully appreciated.

The outcome of this PR:

file system operations for create / rename are asynchronous

code changes are minimal and clear

asynchronous operations are localised i.e. not spread over multiple callbacks / functions

Unless there is a very good reason, inputs should not be asynchronous. This adds unnecessary complexity.

Sorry for that I'm gonna to be off-topic, I think the referred async input codepath would not be ever reached, as we already checked the existence of file before async.exec:

nvim-tree.lua/lua/nvim-tree/actions/fs/create-file.lua

Lines 136 to 142 in c71f47a

if utils.file_exists(new_file_path) then

notify.warn "Cannot create: file already exists"

return

end

if M.enable_async then

async.exec(create_file, new_file_path, function(err)

.
Do you think it's safe to remove this? I just cp from the original implementation. There is no other place with async call to user input.

I see... that's a good catch!

It does look like this file_exits will fail and exit early. The local function create_file(new_file_path) call to file_exists will always pass and the "Overwrite " .. new_file_path .. " ?" prompt cannot occur.

This is existing behaviour: we can safely remove the file_exists check in create_file.

Please raise a separate PR to remove this dead code path. We will merge that first, keeping our changes small and atomic and easily revertable.

I found that I referred the wrong variable, new_file_path -> path_to_create, sorry about that. However, the truth that this is a dead code remains; and I think we should early return instead of prompting for overwrite for already existing file.

alex-courtis · 2023-01-22T02:17:32Z

lua/nvim-tree/async.lua

@@ -0,0 +1,180 @@
+---Idea taken from: https://github.com/ms-jpq/lua-async-await


There's a lot of unused codepaths in here. Let's trim it down. If we do need functionality we can refer to the source.

It looks like:
in_async
wrap
all
interrupt/abort

lua/nvim-tree/async.lua

alex-courtis · 2023-02-04T05:24:45Z

Any updates on this one @yioneko ?

We are getting very close...

alex-courtis · 2023-04-23T04:00:38Z

Closing due to inactivity.

The concept and implementation of async is sound; changes to the usages are required.

yioneko changed the title ~~faet: async rename and create file~~ feat: async rename and create file Dec 31, 2022

yioneko force-pushed the feat-async-fs-op-s1 branch from cc5eda6 to ec32ac2 Compare December 31, 2022 02:18

gegoune requested changes Dec 31, 2022

View reviewed changes

lua/nvim-tree/actions/fs/create-file.lua Outdated Show resolved Hide resolved

lua/nvim-tree/events.lua Outdated Show resolved Hide resolved

yioneko force-pushed the feat-async-fs-op-s1 branch from ec32ac2 to 965e284 Compare December 31, 2022 03:37

alex-courtis changed the base branch from master-reverting to master January 9, 2023 02:43

alex-courtis requested changes Jan 14, 2023

View reviewed changes

alex-courtis mentioned this pull request Jan 14, 2023

feat: make fs actions async #1863

Closed

yioneko force-pushed the feat-async-fs-op-s1 branch from 35c0b43 to e8f7a42 Compare January 20, 2023 02:51

feat: async rename and create file

6e7f5ea

yioneko force-pushed the feat-async-fs-op-s1 branch from e8f7a42 to 6e7f5ea Compare January 20, 2023 02:56

yioneko commented Jan 20, 2023

View reviewed changes

fix: schedule before find file

06f3429

alex-courtis requested changes Jan 22, 2023

View reviewed changes

chore: trim unused async utils

c71f47a

yioneko mentioned this pull request Jan 23, 2023

fix: remove redundant file existence check in create file operation #1936

Merged

alex-courtis mentioned this pull request Feb 6, 2023

Git Execution Blocks UI Thread #1974

Closed

alex-courtis closed this Apr 23, 2023


		M.Interrupter = Interrupter

		---This is useful for cancelling execution async function

	if utils.file_exists(new_file_path) then
	notify.warn "Cannot create: file already exists"
	return
	end

	if M.enable_async then
	async.exec(create_file, new_file_path, function(err)

		@@ -0,0 +1,180 @@
		---Idea taken from: https://github.com/ms-jpq/lua-async-await

Uh oh!

feat: async rename and create file #1870

feat: async rename and create file #1870

Uh oh!

Conversation

yioneko commented Dec 31, 2022

Uh oh!

gegoune left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

alex-courtis commented Jan 2, 2023

Uh oh!

yioneko commented Jan 5, 2023

Uh oh!

alex-courtis commented Jan 7, 2023

Uh oh!

alex-courtis left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

alex-courtis commented Jan 14, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

yioneko commented Jan 14, 2023

Uh oh!

alex-courtis commented Jan 14, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alex-courtis left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yioneko Jan 22, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yioneko Jan 23, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

alex-courtis left a comment •

edited

Loading

alex-courtis commented Jan 14, 2023 •

edited

Loading

alex-courtis left a comment •

edited

Loading

yioneko Jan 22, 2023 •

edited

Loading

yioneko Jan 23, 2023 •

edited

Loading