When exiting vim with :cq, it exits with an error status which is caught
in the termClosed() callback. This causes the composer tab to be closed
and it is a known and expected behaviour.

A recent fix of the tab strip removal revealed that the composer lock is
still held when calling RemoveTab(). Internally, RemoveTab() hides and
unfocuses the removed tab both operations require the composer lock as
well, causing a complete deadlock of the application.

Unfortunately, we cannot rely on the go defer statement to have
RemoveTab(), called *AFTER* c.Unlock(). Deferred statements are called
in the reverse order they were evaluated.

Use an explicit function array to ensure that the composer is unlocked
before RemoveTab() is called.

Fixes: 6b97085ae5ec ("tab: fix broken history on removal")
Signed-off-by: Robin Jarry <robin@jarry.cc>
---
 app/compose.go | 24 ++++++++++++++++++------
 1 file changed, 18 insertions(+), 6 deletions(-)

diff --git a/app/compose.go b/app/compose.go
index 7a6a423c3ea3..e8a672479245 100644
--- a/app/compose.go
+++ b/app/compose.go
@@ -1197,7 +1197,15 @@ func (c *Composer) reopenEmailFile() error {
 
 func (c *Composer) termClosed(err error) {
 	c.Lock()
-	defer c.Unlock()
+	// RemoveTab() on error must be called *AFTER* c.Unlock() but the defer
+	// statement does the exact opposite (last defer statement is executed
+	// first). Use an explicit list that begins with unlocking first.
+	deferred := []func(){c.Unlock}
+	defer func() {
+		for _, d := range deferred {
+			d()
+		}
+	}()
 	if c.editor == nil {
 		return
 	}
@@ -1205,7 +1213,7 @@ func (c *Composer) termClosed(err error) {
 		PushError("Failed to reopen email file: " + e.Error())
 	}
 	editor := c.editor
-	defer editor.Destroy()
+	deferred = append(deferred, editor.Destroy)
 	c.editor = nil
 	c.focusable = c.focusable[:len(c.focusable)-1]
 	if c.focused >= len(c.focusable) {
@@ -1213,8 +1221,10 @@ func (c *Composer) termClosed(err error) {
 	}
 
 	if editor.cmd.ProcessState.ExitCode() > 0 {
-		RemoveTab(c, true)
-		PushError("Editor exited with error. Compose aborted!")
+		deferred = append(deferred, func() {
+			RemoveTab(c, true)
+			PushError("Editor exited with error. Compose aborted!")
+		})
 		return
 	}
 
@@ -1225,8 +1235,10 @@ func (c *Composer) termClosed(err error) {
 			PushError(err.Error())
 			err := c.showTerminal()
 			if err != nil {
-				RemoveTab(c, true)
-				PushError(err.Error())
+				deferred = append(deferred, func() {
+					RemoveTab(c, true)
+					PushError(err.Error())
+				})
 			}
 			return
 		}
-- 
2.47.1

On Mon Jan 13, 2025 at 3:08 PM EET, Robin Jarry wrote:
> When exiting vim with :cq, it exits with an error status which is caught
> in the termClosed() callback. This causes the composer tab to be closed
> and it is a known and expected behaviour.
>
> A recent fix of the tab strip removal revealed that the composer lock is
> still held when calling RemoveTab(). Internally, RemoveTab() hides and
> unfocuses the removed tab both operations require the composer lock as
> well, causing a complete deadlock of the application.
>
> Unfortunately, we cannot rely on the go defer statement to have
> RemoveTab(), called *AFTER* c.Unlock(). Deferred statements are called
> in the reverse order they were evaluated.
>
> Use an explicit function array to ensure that the composer is unlocked
> before RemoveTab() is called.
>
> Fixes: 6b97085ae5ec ("tab: fix broken history on removal")
> Signed-off-by: Robin Jarry <robin@jarry.cc>
> ---
>  app/compose.go | 24 ++++++++++++++++++------
>  1 file changed, 18 insertions(+), 6 deletions(-)
>
> diff --git a/app/compose.go b/app/compose.go
> index 7a6a423c3ea3..e8a672479245 100644
> --- a/app/compose.go
> +++ b/app/compose.go
> @@ -1197,7 +1197,15 @@ func (c *Composer) reopenEmailFile() error {
>  
>  func (c *Composer) termClosed(err error) {
>  	c.Lock()
> -	defer c.Unlock()
> +	// RemoveTab() on error must be called *AFTER* c.Unlock() but the defer
> +	// statement does the exact opposite (last defer statement is executed
> +	// first). Use an explicit list that begins with unlocking first.
> +	deferred := []func(){c.Unlock}
> +	defer func() {
> +		for _, d := range deferred {
> +			d()
> +		}
> +	}()
>  	if c.editor == nil {
>  		return
>  	}
> @@ -1205,7 +1213,7 @@ func (c *Composer) termClosed(err error) {
>  		PushError("Failed to reopen email file: " + e.Error())
>  	}
>  	editor := c.editor
> -	defer editor.Destroy()
> +	deferred = append(deferred, editor.Destroy)
>  	c.editor = nil
>  	c.focusable = c.focusable[:len(c.focusable)-1]
>  	if c.focused >= len(c.focusable) {
> @@ -1213,8 +1221,10 @@ func (c *Composer) termClosed(err error) {
>  	}
>  
>  	if editor.cmd.ProcessState.ExitCode() > 0 {
> -		RemoveTab(c, true)
> -		PushError("Editor exited with error. Compose aborted!")
> +		deferred = append(deferred, func() {
> +			RemoveTab(c, true)
> +			PushError("Editor exited with error. Compose aborted!")
> +		})
>  		return
>  	}
>  
> @@ -1225,8 +1235,10 @@ func (c *Composer) termClosed(err error) {
>  			PushError(err.Error())
>  			err := c.showTerminal()
>  			if err != nil {
> -				RemoveTab(c, true)
> -				PushError(err.Error())
> +				deferred = append(deferred, func() {
> +					RemoveTab(c, true)
> +					PushError(err.Error())
> +				})
>  			}
>  			return
>  		}

Works as expected. Thanks!

Tested-by: Julio B <julio.bacel@gmail.com>

Robin Jarry <robin@jarry.cc> wrote:
> When exiting vim with :cq, it exits with an error status which is caught
> in the termClosed() callback. This causes the composer tab to be closed
> and it is a known and expected behaviour.
>
> A recent fix of the tab strip removal revealed that the composer lock is
> still held when calling RemoveTab(). Internally, RemoveTab() hides and
> unfocuses the removed tab both operations require the composer lock as
> well, causing a complete deadlock of the application.
>
> Unfortunately, we cannot rely on the go defer statement to have
> RemoveTab(), called *AFTER* c.Unlock(). Deferred statements are called
> in the reverse order they were evaluated.
>
> Use an explicit function array to ensure that the composer is unlocked
> before RemoveTab() is called.
>
> Fixes: 6b97085ae5ec ("tab: fix broken history on removal")
> Signed-off-by: Robin Jarry <robin@jarry.cc>
> ---

Tested-by: Julio B <julio.bacel@gmail.com>

Applied, thanks.

To git@git.sr.ht:~rjarry/aerc
   fa0b99800ee7..7766b4b3334f  master -> master