Adding timeout when fetching plugin & more clear plugin error message #3389
Conversation
internal/config/plugin_installer.go
Outdated
| if err != nil { | ||
| fmt.Fprintln(out, "Failed to query plugin repository:\n", err) | ||
| fmt.Fprintln(out, "Failed to query plugin repository", pr) |
There was a problem hiding this comment.
Errors returned when retrieving plugins and channels are printed so I do not see why it would not be done with repositories.
There was a problem hiding this comment.
Wrong edit when testing, corrected it back. Thanks
internal/config/plugin_installer.go
Outdated
| if err != nil { | ||
| fmt.Fprintln(out, "Failed to query plugin channel:\n", err) | ||
| fmt.Fprintln(out, "Failed to query plugin channel ", pc, " with error:\n", err) |
There was a problem hiding this comment.
Space is printed between arguments when calling fmt.Fprintln so space does not have to be added at start or end of arguments. I tried testing a bit using Go 1.15 and 1.19 but URL was included in the error when running server using OpenBSD nc where nothing is sent. Are there errors where URL is not included?
It may also be already clear that messages like this are errors even if "with error" is not written:
Failed to query plugin channel:
Get "http://localhost:8080": context deadline exceeded (Client.Timeout exceeded while awaiting headers)
There was a problem hiding this comment.
Yep you are right, I have reverted the changes back, thanks.
Neko-Box-Coder
force-pushed
the
PluginTimeout
branch
2 times, most recently
from
389d446
to
2048d69
Compare
Neko-Box-Coder
force-pushed
the
PluginTimeout
branch
from
2048d69
to
61ec1c3
Compare
|
Taking #3413 into account isn't a timeout of 10s still a bit too much, before any further user action is possible? |
|
Ideally, I would want an UI feedback when downloading or something, but that would probably require async or something like that (Both with command line interface and inside the editor). You are right 10 seconds could be a bit too short. Maybe I can put the Or should I just increase the timeout to like 20 or 30 seconds for now? |
Not in case the user can't do anything in this moment. Waiting for 10s, without the possibility to proceed, could already being a long time. Maybe the user could be ask if a retry shall take place with possible increased (duplicated timeout), because he should somehow know is connectivity? |
|
We can make the timeout arbitrarily long, as long as the user can easily interrupt it. That requires more work to implement (to do the http request in a separate goroutine, cancel the http request, integrate it with the infobar event loop etc), but it seems worth doing? (For the command-line case we don't need to do that.) |
|
That's another option. |
|
I didn't mean running it in the background while the user can do something else in the UI. I meant running it in the "foreground" from the user perspective, like we do now, but with something like "Downloading list of plugins, press Esc to interrupt". |
|
Ok, that sounds like the way to go and worth the effort. 👍 |
|
Yeah... The suggestion is good but I think someone else maybe better off implementing it than me given I am not that familiar with the codebase and language 😅. golang is not my main language after all The purpose of this PR is just to add a timeout (so that it can't hang indefinitely) and a better error message when failing to parse json (so that the user know which repo/channel has faulty json) Maybe we can (finalize and) merge this in for now and someone can create a PR for the suggestion? |
|
I have increased the timeout to 30 seconds, which should be enough for the majority of the plugins even if the internet is quite slow. Also added a message to tell the user to wait before we do any fetching. I would rather it hang for 30 seconds (for the time being) than getting hung forever, which would be bad if the user has any unsaved changes when doing plugin update in the editor |
Currently the
http.Get()can hang for a long time, which also has no user feedback when doing something likemicro -plugin availableAdding a timeout can tell the user which repo/channel is taking a long time, hence be able to inform the maintainer or troubleshoot it.
Also changed the error message for plugins to tell which part has failed to parse, which makes it easier to troubleshoot as well