Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Execute multiply nested SequenceMsg and BatchMsg in the proper order. #847

Open
wolfmagnate opened this issue Oct 23, 2023 · 5 comments
Open
Labels
enhancement New feature or request

Comments

@wolfmagnate
Copy link

wolfmagnate commented Oct 23, 2023

Is your feature request related to a problem? Please describe.

Description

As stated in issue #680, the current eventLoop method handling for sequenceMsg executes only once nested BatchMsg in the proper order, but nested sequenceMsg and multiple nested BatchMsg or sequenceMsg will not follow the specified order.
This is due to the fact that the eventLoop method calls p.Send without checking the result of the message that is the result of the call to the command contained in sequenceMsg all the way through the nest.

Example

The following shows the desired and actual calling order for a given sequenceMsg.

func (m model) Init() tea.Cmd {
	return tea.Sequence(
		// Command1
		tea.Batch(
			// Command1-1
			tea.Sequence(
				tea.Println("1-1-1"),
				tea.Println("1-1-2")
			),
			// Command1-2
			tea.Batch(
				tea.Println("1-2-1"),
				tea.Println("1-2-2")
			)
		),
		// Command2
		tea.Println("2"),
		// Command3
		tea.Sequence(
			// Command3-1
			tea.Batch(
				tea.Println("3-1-1"),
				tea.Println("3-1-2")
			),
			// Command3-2
			tea.Sequence(
				tea.Println("3-2-1"),
				tea.Println("3-2-2")
			)
		),
		// last Command
		tea.Quit,
	)
}

These are important points of the example above.

  • Command1 is generated by tea.Batch, so Command1-1 and Command1-2 are executed in parallel.
  • Command1-1 is generated by tea.Sequence, so Command1-1-1 and Command1-1-2 are executed in sequence.
  • Command1-2 is generated by tea.Batch, so Command1-2-1 and Command1-2-2 are executed in parallel.
  • Command2 is executed after all commands executed by Command1 (1-1-1, 1-1-2, 1-2-1, 1-2-2) have been executed.
  • Command3 is executed after Command2 finishes.
  • Command3 is generated by tea.Sequence, so Command3-1 and Command3-2 are executed in sequence.
  • Command3-1 is generated by tea.Batch, so Command3-1-1 and Command3-1-2 are executed simultaneously.
  • Command3-2 is generated by tea.Sequence, so Command3-2-1 and Command3-2-2 are executed in sequence.
  • Quit is executed after all these commands are completed.

This is a simple sequence diagram for Command1.
In the figure below, the solid black line represents the go routine, the solid red line represents the invocation of the go routine, and the dashed red line represents the end of the go routine being notified to the go routine that invoked the go routine just finished.

cmd1 drawio

This is a simple sequence diagram for Command3.
cmd3 drawio

In the current implementation, the above commands are executed as shown below.

badCmd1 drawio
In this section of tea.go, the bubble tea works like sequence diagram above.
The reason processing is not executed in the specified order is that eventloop is asked to execute all cmd()'s results asynchronously using p.Send() without checking whether they are sequenceMsg or BatchMsg or other Messages.

The problem and its cause when Cmd3 is executed with the current implementation is discussed in #680 and #843. Briefly, when sequenceMsg is nested in sequenceMsg, the inner sequenceMsg is sent to eventloop with p.Send, so it cannot maintain proper order with the outer sequenceMsg.

Describe the solution you'd like

improve implementation of #843. To execute the command as specified for a sequenceMsg or BatchMsg of any depth of nesting, the command should be executed using a recursive function.

Describe alternatives you've considered

Since the current implementation supports nesting by directly examining the result of the command invocation and nesting if statements instead of using a recursive function, the if statements must also be nested by the corresponding depth of nesting.
For now, I have not come up with an alternative.

Additional context

breaking change

Consideration should be given so that this change is not breaking. This modification changes the order in which the Command is invoked. However, in the current implementation, Command was simply executed asynchronously with p.Send without adhering to the order specified by tea.Sequence or tea.Batch. Therefore, the commands in the program that work correctly at this time are not guaranteed to be in sequence. In other words, the program works even if the order is not guaranteed. Therefore, even if this modification guarantees order, it is unlikely that a program that was already working will stop working. On the other hand, the programs that will not work correctly due to this modification are programs with bugs that happen to be working well due to the unguaranteed execution order by p.Send and eventloop.

other information

@wolfmagnate
Copy link
Author

I am not sure this issue is a new feature addition. This might be a bug fix.

@wolfmagnate
Copy link
Author

How to reproduce the problem

Change Init in examples/sequence/main.go and add SleepPrintln like this.

func (m model) Init() tea.Cmd {
	return tea.Sequence(
		tea.Batch(
			tea.Sequence(
				SleepPrintln("1-1-1", 1000),
				SleepPrintln("1-1-2", 1000),
			),
			tea.Batch(
				SleepPrintln("1-2-1", 1500),
				SleepPrintln("1-2-2", 1250),
			),
		),
		tea.Println("2"),
		tea.Sequence(
			tea.Batch(
				SleepPrintln("3-1-1", 500),
				SleepPrintln("3-1-2", 1000),
			),
			tea.Sequence(
				SleepPrintln("3-2-1", 750),
				SleepPrintln("3-2-2", 500),
			),
		),
		tea.Quit,
	)
}

// print string after stopping for a certain period of time
func SleepPrintln(s string, milisecond int) tea.Cmd {
	printCmd := tea.Println(s)
	return func() tea.Msg {
		time.Sleep(time.Duration(milisecond) * time.Millisecond)
		return printCmd()
	}
}

If you execute sequence with the current implementation of tea.go, you will get the following.

$ go run main.go
2

This is because tea.Quit is executed before SleepPrintln is done.

However, with #848 the program works well.

$ go run main.go
1-1-1 // after 1000ms
1-2-2 // after 1250ms
1-2-1 // after 1500ms
1-1-2 // after 2000ms
2
3-1-1 // after 2500ms
3-1-2 // after 3000ms
3-2-1 // after 3750ms
3-2-2 // after 4250ms

@luevano
Copy link

luevano commented Jun 19, 2024

So is this issue dead? just ran into this and spent a lot of time trying to figure out what was happening. And I was just using a nested Sequence, not even Batch cmds.

Tried #848 and it worked fine for my case.

@wolfmagnate
Copy link
Author

wolfmagnate commented Jun 19, 2024

Oh, I totally forgot this PR! Yes I think this should be solved.

@wolfmagnate
Copy link
Author

@meowgorithm Could you check this issue about SequenceMsg and BatchMsg? I am willing to resolve conflict of #848

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

2 participants