Add query server command to expose internal state
#1310
Conversation
There was a problem hiding this comment.
Looks nice, this solves many of the problems with the other approaches.
My one concern about this approach is that it creates a performance vs reliability tradeoff I discuss in another comment. Though, perhaps, something about the API we are using means that talking to the server is guaranteed to be quick?
Some really minor thoughts:
-
This approach does mean that
:mapsdo not work if you startlfaslf -single. That's probably fine; it seems like less of a problem to me than the issues with the other approaches. -
If anybody is testing this, don't forget to
killall lfregularly. Otherwise, you're likely to have anlfclient with this PR communicate with a server that doesn't have this PR, which results in confusing errors.
client.go
Outdated
| c.Close() | ||
|
|
||
| // wait for server to finish storing data and send response, if any | ||
| io.ReadAll(c) |
There was a problem hiding this comment.
I haven't seen any problems yet, but there is a potential performance problem here. If I understand the code correctly (following your comment), if sending the data is slow over the connection, lf would hang, and it looks like this would happen after every command. Might this happen if the connection is interrupted somehow, as well? Would it then hang forever or until some timeout? I'm unsure.
Perhaps there is some reason this is not a concern?
If it is a concern, one way to fix it would be to make this happen in a goroutine and not wait for success before executing the following commands. I think that's be OK with the data we're currently sharing this way -- maps and cmds rarely change, and when they do, presenting an old version is not the end of the world.
However, going beyond the scope of this PR, not waiting for confirmation would make it extremely dangerous to store things like $fs in this way. At first, I was excited about the prospect of addressing something like #47 using query. However, it would be very problematic if somebody ran lf -remote "query $id fs"|xargs -0 rm and got an old version of a file selection. So, if we go that way, I think we should put a prominent warning in the code (in exportData, say) to not share such information via 'query'.
There was a problem hiding this comment.
If the connection is slow then yes lf would hang until it's finished or there's some kind of error. The server doesn't really do anything much apart from handling connections and forwarding send commands, so from a performance point of view it is sufficiently fast and I don't notice any kind of delay unless I deliberately add in time.Sleep for testing purposes.
Storing the data in the same thread and waiting for the server to finish was my intention, otherwise there could be a race condition as you have described. Currently app.runShell continues anyway even storing the data fails (e.g. if I deliberately kill the server), do you think it would be better to return instead?
There was a problem hiding this comment.
I'm unsure what's best. For the data we are storing now, I'm OK with race conditions as I said. If we, theoretically, stored something like the selection this way, I'd want app.runShell to abort if anything is off.
I've never noticed performance problems with lf -remote commands, so maybe that's OK. I do run into #495; I don't understand that issue thoroughly, but I think you'd know whether it would be a problem here.
There was a problem hiding this comment.
I'm generally allowing race conditions - although maps and cmds rarely change, the jump list will naturally accumulate entries. I have added another commit c55607a which checks if the store command succeeds, otherwise app.runShell is aborted, although in my testing I haven't encountered this scenario without deliberately killing the server.
#495 is a separate issue which only involves running lf -remote during startup - I posted a more detailed explanation here: #1293 (comment). That being said you can trigger a similar error if you add maps as a startup command, but I can't see why anyone would want to do this.
@ilyagr Good point about |
I would, personally, rather switch to the approach of #1306 or get rid of the We could also try to make it so that |
OK at this stage, you have somewhat convinced me that this approach also has some drawbacks too, when compared to the At the end of the day, I'm fine with either approach, I just wanted to make sure different strategies are investigated, because I don't want to introduce a feature like this, only to find that there is something better but also can't change because people are already using it. |
|
For now I will move this PR back to a draft as well. Maybe some further discussion is needed, perhaps with input from @gokcehan. While I am fine with this approach being reliant on the server, I don't like the fact that default commands like |
|
I'm also not sure what's best, but I think we have two pretty good options now. Thank you again for working on them! |
|
@joelim-work Thank you very much for coming up with yet another simpler approach for this purpose. I have been playing with it for a while and it seems to work well. I think my initial concerns are similar to @ilyagr 's in that I'm also not sure if the server involvement in every command is a good idea or not, but I can't think of a good solution at this moment. Maybe a command type modifier can be added similar to what is proposed in #1277 (e.g. |
|
@gokcehan The main And a user option This does make the default commands (i.e. On a side note, I have found a way to add color when mappings/other info is displayed, by piping the result of |
|
@joelim-work I played with the code a little bit last night but eventually I gave up. Since the main thread is blocked, I tried to utilize the goroutine created for the remote commands (i.e. Adding an explicit |
|
@gokcehan I don't think I mentioned it anywhere, but I also previously tried having the server forward the query to the main instance and then back to the I guess implementing the lookup in Anyway I implemented the
It does add a bit more complexity compared to just storing by default (effectively |
|
@joelim-work It may not worth trying since it does not work but I had tried something along the following: Click to show diffdiff --git a/client.go b/client.go
index 0fab7d5..55f23e2 100644
--- a/client.go
+++ b/client.go
@@ -7,6 +7,7 @@ import (
"log"
"net"
"os"
+ "strconv"
"strings"
"time"
@@ -84,9 +85,45 @@ func readExpr() <-chan expr {
s := bufio.NewScanner(c)
for s.Scan() {
log.Printf("recv: %s", s.Text())
- p := newParser(strings.NewReader(s.Text()))
- if p.parse() {
- ch <- p.expr
+ word, rest := splitWord(s.Text())
+ switch word {
+ case "send":
+ if rest != "" {
+ word2, rest2 := splitWord(rest)
+ id, err := strconv.Atoi(word2)
+ if err != nil {
+ p := newParser(strings.NewReader(rest))
+ if p.parse() {
+ ch <- p.expr
+ }
+ } else {
+ if id != gClientID {
+ log.Println("send: wrong id")
+ } else {
+ p := newParser(strings.NewReader(rest2))
+ if p.parse() {
+ ch <- p.expr
+ }
+ }
+ }
+ }
+ case "recv":
+ if rest != "" {
+ word2, rest2 := splitWord(rest)
+ id, err := strconv.Atoi(word2)
+ if err != nil {
+ log.Println("recv: only single id is supported")
+ } else {
+ if id != gClientID {
+ log.Println("recv: wrong id")
+ } else {
+ if rest2 == "maps" {
+ fmt.Fprintln(c, listBinds(gOpts.keys).String())
+ }
+ // TODO: add the rest
+ }
+ }
+ }
}
}
diff --git a/server.go b/server.go
index 7d63ca6..5060bb3 100644
--- a/server.go
+++ b/server.go
@@ -3,6 +3,7 @@ package main
import (
"bufio"
"fmt"
+ "io"
"log"
"net"
"os"
@@ -102,20 +103,35 @@ Loop:
}
case "send":
if rest != "" {
- word2, rest2 := splitWord(rest)
+ word2, _ := splitWord(rest)
id, err := strconv.Atoi(word2)
if err != nil {
for _, c := range gConnList {
- fmt.Fprintln(c, rest)
+ fmt.Fprintln(c, s.Text())
}
} else {
if c2, ok := gConnList[id]; ok {
- fmt.Fprintln(c2, rest2)
+ fmt.Fprintln(c2, s.Text())
} else {
echoerr(c, "listen: send: no such client id is connected")
}
}
}
+ case "recv":
+ if rest != "" {
+ word2, _ := splitWord(rest)
+ id, err := strconv.Atoi(word2)
+ if err != nil {
+ echoerr(c, "listen: recv: only single id is supported")
+ } else {
+ if c2, ok := gConnList[id]; ok {
+ fmt.Fprintln(c2, s.Text())
+ io.Copy(c, c2)
+ } else {
+ echoerr(c, "listen: recv: no such client id is connected")
+ }
+ }
+ }
case "quit":
if len(gConnList) == 0 {
gQuitChan <- struct{}{}I agree this may not be a good idea due to potential race conditions. I will take a look at your new branch. |
|
Hi @gokcehan I think the server doesn't distinguish between connections from This also reminds me of the discussion in #1314, where global state is shared among different threads. |
|
Actually maybe ignore the above comment from me, I managed to get the Click to show diffdiff --git a/client.go b/client.go
index 0fab7d5..cbc9ef2 100644
--- a/client.go
+++ b/client.go
@@ -84,9 +84,23 @@ func readExpr() <-chan expr {
s := bufio.NewScanner(c)
for s.Scan() {
log.Printf("recv: %s", s.Text())
- p := newParser(strings.NewReader(s.Text()))
- if p.parse() {
- ch <- p.expr
+ if word, rest := splitWord(s.Text()); word == "recv" {
+ switch rest {
+ case "maps":
+ io.Copy(c, listBinds(gOpts.keys))
+ case "cmaps":
+ io.Copy(c, listBinds(gOpts.cmdkeys))
+ case "cmds":
+ io.Copy(c, listCmds())
+ case "jumps":
+ // TODO: not available globally
+ }
+ fmt.Fprintln(c, "")
+ } else {
+ p := newParser(strings.NewReader(s.Text()))
+ if p.parse() {
+ ch <- p.expr
+ }
}
}
diff --git a/server.go b/server.go
index 7d63ca6..2e2a1a1 100644
--- a/server.go
+++ b/server.go
@@ -84,6 +84,7 @@ Loop:
echoerr(c, "listen: conn: client id should be a number")
} else {
gConnList[id] = c
+ return
}
} else {
echoerr(c, "listen: conn: requires a client id")
@@ -95,6 +96,9 @@ Loop:
if err != nil {
echoerr(c, "listen: drop: client id should be a number")
} else {
+ if c2, ok := gConnList[id]; ok {
+ c2.Close()
+ }
delete(gConnList, id)
}
} else {
@@ -116,6 +120,29 @@ Loop:
}
}
}
+ case "recv":
+ if rest != "" {
+ word2, rest2 := splitWord(rest)
+ id, err := strconv.Atoi(word2)
+ if err == nil {
+ if c2, ok := gConnList[id]; ok {
+ fmt.Fprintln(c2, "recv "+rest2)
+ s2 := bufio.NewScanner(c2)
+ for s2.Scan() {
+ if s2.Text() == "" {
+ break
+ }
+ fmt.Fprintln(c, s2.Text())
+ }
+ } else {
+ echoerr(c, "listen: recv: no such client id is connected")
+ }
+ } else {
+ echoerr(c, "listen: recv: client id should be a number")
+ }
+ } else {
+ echoerr(c, "listen: recv: requires a client id")
+ }
case "quit":
if len(gConnList) == 0 {
gQuitChan <- struct{}{}The trick was to just simply prevent the server from trying read from the main instance after registering it by adding a I'm still concerned about thread safety, and also the jump list isn't available globally. I'm not sure what's the best way to tackle this, but maybe it's enough to just have I think this approach might be more attractive than having to worry about making repeated calls to the server to store data, so I'm willing to keep this idea in consideration. I have pushed my work up to this branch: https://github.com/joelim-work/lf/tree/remote-recv |
|
@joelim-work Maybe I forgot to drop a reply here, sorry about that. I think both approaches are fine. I had tried them both briefly before and I had not noticed any issues. If you have a preference already and you think the patch is ready, feel free to merge it. You don't have to wait for my approval. |
|
Closing in favor of #1384. Might leave it open for a few days or so if anyone wants to have a look. |
Description
This PR supersedes #1306 and is an alternate approach of solving the problem of exposing internal state for scripting purposes.
This also fixes a bug in Windows where
cmaps/cmds/jumpsexits immediately without showing anything because that is the behaviour of themorepager if there isn't enough content to fill an entire page. The reason is thatrunPagerOnis limited to using$commands, butlf -remote "query $id ..."can be used in any type of shell command.Examples
Display mappings:
Change location based on the jump list:
Use
awkto add color when displaying mappings and other info:Notes
runPagerOnfunctionality. This works on Windows as well, provided Improve windows shell handling #1309 is merged.