Error() freezes program when called inside makeNewTab -> t.view.SetInputCapture

[mntn-xyz]

Found this because my error messages for #265 started freezing when I merged the latest code from master.

To reproduce, add the following anywhere inside t.view.SetInputCapture (in display/tab.go):

Error("Test Error", "Test")

For instance, add the above code when the copy key is pressed so it will always have an error when called. Amfora will freeze completely if you try to copy a URL.

I assume this means that the current copy and save errors will freeze the program if they are encountered, but I'm not sure how to trigger those intentionally.

Confirmed that this was introduced in #272.

[mntn-xyz]

Drop-in example (display/tab.go):

201       err = clipboard.WriteAll(copiedURL.String())                                                                                                                                                      
202       if err != nil {                                                                                                                                                                                   
203         Error("Copy Error", err.Error())                                                                                                                                                                
204         return nil                                                                                                                                                                                      
205       }                                                                                                                                                                                                 
206 +     Error("Test Error", "Test")
207       return nil
208     }

[awfulcooking]

Hi @mntn-xyz, yes, this is my fault.

I originally made Error() return a channel that could be waited on, but discovered there are a lot of calls that need to wait in display/handlers.go, and chose to make it block instead.

They needed to wait on the modal being dismissed, so that when their parent callers got wind of the error, their activity wouldn't defocus the modal & make it undismissable. In this case, navigating to about:newtab to hide the loading message.

I figured there could be a lot of scenarios like that & so it was logical to make Error() block.

Shortly after it was merged, I found this freeze which took some time to debug. So I empathize with you.

It seems that CustomCommand() sits in a similar place to URL() .. which is called synchronously from UI handlers, but spawns a goroutine to do its navigation so the main loop is free from that point on.

There are a number of other approaches we could take but I'm not sure any of them are simpler or more foolproof...

What do you think?

[mntn-xyz]

Thanks for looking into it. Goroutines are probably the right way to do this kind of thing anyway, so overall this change will improve the code. I think it's probably wise to check all the UI code for calls to blocking functions just in case something has slipped by.

I will see if I can get a patch together as soon as I have a minute, but don't wait on me if you're already working on something :)

[awfulcooking]

Move fast and break things

:)

[makew0rld]

Look at this, I don't even need to respond to issues anymore! 😄 Thanks for explaining @awfulcooking.

To confirm I understand the situation:

• Error blocks when called, until it's dismissed
• If called in the main thread, Amfora freezes totally, because it blocks the user from being able to interact with the UI to dismiss the error modal
• The solution is to call the function that calls Error in a goroutine, or the function that calls that function, etc

I think it's probably wise to check all the UI code for calls to blocking functions just in case something has slipped by.

I agree. You've already found the issue with copying, thanks for that. Assuming @awfulcooking isn't already working on it, @mntn-xyz making a fix for this would be great, thank you.

The solution for the copy case is probably just to call go Error(... right? Since there's not encapsulating function and you don't want to create any race conditions.

Move fast and break things

:)

Plz no lol

[awfulcooking]

I'm wondering if it's the right approach. Let me jot a couple of alternatives / see how they strike you:

1. Make Error() return a channel, which may optionally be waited on. The calls downstream from navigation (a couple of dozen in handlers.go) would need to change from "Error(...)" to "<-Error(...)" so that the loading page can go away at the right time. But given nothing else yet needs to wait, other stuff should be safe.
2. Lean in on Error() blocking, and update the remaining calls (clipboard, save page, followLink(), one or two other errors). ??? Profit
3. Other. e.g. make URL() wait for any modals to be dismissed before navigating again. Unmerge the loading page and forget this whole thing happened. Pray to the sun god Ra, etc

Hoping for consistency between Info(), Error() and any future modal calls, and a lowish chance of similar bugs in future, what are your thoughts?

Plz no lol

I take responsibility for all these modals breaking, and I'll offer a patch whatever we choose. I'll try to be more careful lol

[mntn-xyz]

I looked at this a bit and just calling Error as a goroutine isn't a great solution after all. There are some places where it doesn't make sense to do this, as it's possible to accidentally create a situation where the error modal loses focus and gets stuck on the screen forever. From a maintenance perspective one of these other options is probably better. I also agree that there should be some consistency between the different modals. Maybe it should block after all? I'm not sure if anything in amfora really needs the operation to continue while an error is displayed.

[makew0rld]

So ideally all modals could not be overrided by other key bindings or code, with the important exception of the quit command. I think that should be the main goal of all this. When a modal pops up, it must be acted on before anything else can happen. Except quitting.

This makes it seem like solution no. 2 as proposed by @awfulcooking is the correct one.

Not sure what should be done about the case of the copy command though? Because any errors there should block other commands (except quit!) but not block the UI. But there's no function going on that can be put in a goroutine.

[makew0rld]

Related: #283

[awfulcooking]

Want to revert #272 until this is resolved?

Given that Info() will have to change too (#283), I think it'd be good to let the solution gestate for a bit before release. Thanks for being open to it :-)

[makew0rld]

Well #272 is already in a release, v1.9.0. I'm not keen to revert it, I think that would complicate things further. Let's figure this out and fix problems as they arise. Let me know if you have an idea about my question above, and what the best option is overall.

[awfulcooking]

Cool, ack and agreed