Set --embed flag by default to avoid hanging on RPC calls #161
Conversation
…s default behavior.
This comment was marked as off-topic.
This comment was marked as off-topic.
9Y5
changed the title
| } | ||
| } | ||
| if !cpos.disableEmbed { | ||
| cpos.logf("[go-client/nvim] Warning: '--embed' flag missing, appending by default. It enables RPC calls via stdin/stdout. To disable this behavior, add ChildProcessDisableEmbed(). More info: https://neovim.io/doc/user/starting.html#--embed") |
There was a problem hiding this comment.
Hmm. I guess we can't make this a fatal error, because that would be backwards-incompatible. So I guess it makes sense to only warn.
There was a problem hiding this comment.
Actually, adding --embed is also backwards-incompatible. So maybe this should be a fatal error.
There was a problem hiding this comment.
Hey @justinmk this is a good discussion to have. To arrive at this approach, I had to make some assumptions on how often --embed is being used. Sharing my analysis here:
| Method | Description | Backward-incompatible? i.e. does it break applications that don't want --embed? |
Comments |
|---|---|---|---|
| Method 1: Log Fatal |
Hard exit, then recommend user to include --embed in their NewChildProcess() options |
Yes | Tells user to fix their params to include --embed. If --embed is really not needed, they can override this panic by adding ChildProcessDisableEmbed() |
| Method 2: Log Warn |
Soft warning, without setting --embed |
No | This just warns but doesn't append a default. This is a safe approach, but also (imo) the least helpful. |
| Method 3: Log Warn and set Default |
Soft warning, append a sensible default --embed |
Yes | Assumes that --embed is the likely behavior that user wants, so adds it by default. If --embed is really not needed, user can disable it with ChildProcessDisableEmbed(). |
I first eliminated Method 2 because a soft warning, although backward-compatible, is probably not what user wants. I assumed that --embed is needed in most setups. And so even if users upgrade to this version, it doesn't fix anything except produce a warning, which isn't as useful as the other 2 methods.
So Method 1 and Method 3 attempts to make it useful by assuming --embed is a sensible default for most cases. Both methods will be backwards-incompatible because it assumes --embed as the preferred behavior. Both methods give a flag ChildProcessDisableEmbed() to opt out from --embed if this assumption is false.
The difference is:
-
Method 1 panics and educates users to fix their
NewChildProcess()to include the--embedargs in their setup. -
Method 3 warns and sets the default to include
--embed.
Good thing about Method 1 is it's explicit, however, it continues to make the go-client harder to setup/use because users will need to know that --embed is needed to make RPC calls. But as we saw from the related Issue #104 , it's actually not obvious that --embed flag needs to be passed.
I finally went with Method 3 as it can simplify the usage of go-client by setting --embed by default. This means users don't need to know about the existence of --embed to use the API.
Open to refine the solution, and curious to know how other libraries are using go-client in the wild 😄
There was a problem hiding this comment.
Good thing about Method 1 is it's explicit, however, it continues to make the
go-clientharder to setup/use because users will need to know that--embedis needed to make RPC calls.
This doesn't seem proportionate. go-client users need to know about nvim flags and RPC concepts in general. An explicit error fixes 90% of the issue, right?
But as we saw from the related Issue #104 , it's actually not obvious that
--embedflag needs to be passed.
The error will make it obvious, though. 😄
There was a problem hiding this comment.
go-client users need to know about nvim flags and RPC concepts in general. An explicit error fixes 90% of the issue, right?
I agree that explicit error (Method 1) will make the developer-error obvious. It is also an improvement. That's why it was considered as a viable solution together with Method 3.
Consider this: there are many nvim flags to consider, and Method 3 makes go-client easier to use by setting a sensible value --embed as the default. For example, if we know that --embed is set for 90% of the use-cases, then we've made the library easier to use for most of our go-client users.
Note that this change doesn't affect the go-client authors who know/remembered about the existence of --embed, since they would have instantiated the child process with ChildProcessArguments("--embed", ...) in their applications already.
There was a problem hiding this comment.
For example, if we know that
--embedis set for 90% of the use-cases, then we've made the library easier to use for most of our go-client users.
True. But this is more a concern for Nvim, not its clients.
Also in favor of "fatal error": If we make it an error now, we can always relax that in the future. But the inverse is not true.
There was a problem hiding this comment.
Mmm, I suppose so. In that case, I'm ok to go with either two methods then.
One final concern: Do you think there are go-client users who don't need --embed? such that if we go with Method 1 now, then performing log fatal will break their libraries?
There was a problem hiding this comment.
I'll comment about my opinions for this thread later (Now JST is 2:00 AM :P)
Co-authored-by: Justin M. Keyes <justinkz@gmail.com>
justinmk
changed the title
Identified that missing
--embed flagwas causing RPC calls tonvim_exec_luato hang. See #155Changes:
--embedflag by default in thenvim.NewChildProcessfunction to ensure a natural default to allow RPC calls to function correctly.--embedby setting thenvim.ChildProcessDisableEmbed()option.Close: #104