Revert "Replace stop callbacks with contexts"

This reverts commit 410ac73469. The rationale is that the frontend can wrap its own components with a disposable thread-safe wrapper that doesn't work once it's invalidated if it wants the guarantee that the component doesn't work anymore once the context is stopped.
Make SetterMethods ContainerUpdaterMethods
2021-05-04 16:19:13 -07:00 · 2021-05-01 21:41:39 -07:00 · 2021-05-01 17:49:25 -07:00 · 2021-05-01 17:22:01 -07:00
5 changed files with 64 additions and 48 deletions
--- a/cchat.go
+++ b/cchat.go
@ -318,12 +318,10 @@ type Completer interface {
 }

 // Configurator is an interface which the backend can implement for a primitive
-// configuration API. Since these methods do return an error, they are allowed
-// to do IO. The frontend should handle this appropriately, including running
-// them asynchronously.
+// configuration API.
 type Configurator interface {
-	SetConfiguration(context.Context, map[string]string) error // Blocking
-	Configuration(context.Context) (map[string]string, error)  // Blocking
+	SetConfiguration(map[string]string) error
+	Configuration() map[string]string
 }

 // Editor adds message editing to the messenger. Only EditMessage can do IO.
@ -357,7 +355,7 @@ type Identifier interface {
 // Labels given to the frontend may contain images or avatars, and the frontend
 // has the choice to display them or not.
 type LabelContainer interface {
-	SetLabel(text.Rich)
+	SetLabel(context.Context, text.Rich)
 }

 // ListMember represents a single member in the member list. Note that this
@ -454,7 +452,7 @@ type MemberDynamicSection interface {
 type MemberListContainer interface {
 	// RemoveMember removes a member from a section. If neither the member nor the
 	// section exists, then the client should ignore it.
-	RemoveMember(sectionID ID, memberID ID)
+	RemoveMember(ctx context.Context, sectionID ID, memberID ID)
 	// SetMember adds or updates (or upsert) a member into a section. This operation
 	// must not change the section's member count. As such, changes should be done
 	// separately in SetSection. If the section does not exist, then the client
@ -464,12 +462,12 @@ type MemberListContainer interface {
 	// Typically, the backend should try and avoid calling this method and instead
 	// update the labeler in the name. This method should only be used for adding
 	// members.
-	SetMember(sectionID ID, member ListMember)
+	SetMember(ctx context.Context, sectionID ID, member ListMember)
 	// SetSections (re)sets the list of sections to be the given slice. Members from
 	// the old section list should be transferred over to the new section entry if
 	// the section name's content is the same. Old sections that don't appear in the
 	// new slice should be removed.
-	SetSections(sections []MemberSection)
+	SetSections(ctx context.Context, sections []MemberSection)
 }

 // MemberLister adds a member list into a message server.
@ -539,12 +537,12 @@ type MessageUpdate interface {
 // allowed to have multiple views. This is usually done with tabs or splits, but
 // the backend should update them all nonetheless.
 type MessagesContainer interface {
-	DeleteMessage(MessageDelete)
-	UpdateMessage(MessageUpdate)
+	DeleteMessage(context.Context, MessageDelete)
+	UpdateMessage(context.Context, MessageUpdate)
 	// CreateMessage inserts a message into the container. The frontend must
 	// guarantee that the messages are in order based on what's returned from
 	// Time().
-	CreateMessage(MessageCreate)
+	CreateMessage(context.Context, MessageCreate)
 }

 // Messenger is for servers that contain messages. This is similar to Discord or
@ -620,10 +618,10 @@ type ReadContainer interface {
 	// their read indicators. The backend can use this to free up users/authors that
 	// are no longer in the server, for example when they are offline or have left
 	// the server.
-	DeleteIndications(authorIDs []ID)
+	DeleteIndications(ctx context.Context, authorIDs []ID)
 	// AddIndications adds a map of users/authors to the respective message ID of
 	// the server that implements ReadIndicator.
-	AddIndications([]ReadIndication)
+	AddIndications(context.Context, []ReadIndication)
 }

 // ReadIndicator adds a read indicator API for frontends to show. An example of
@ -723,7 +721,7 @@ type ServerUpdate interface {
 // as servers can be infinitely nested. Frontends should also reset the entire
 // node and its children when SetServers is called again.
 type ServersContainer interface {
-	UpdateServer(ServerUpdate)
+	UpdateServer(context.Context, ServerUpdate)
 	// SetServer is called by the backend service to request a reset of the server
 	// list. The frontend can choose to call Servers() on each of the given servers,
 	// or it can call that later. The backend should handle both cases.
@ -733,7 +731,7 @@ type ServersContainer interface {
 	// should only be considered empty if it's an empty non-nil slice. An
 	// unavailable list, on the other hand, can be treated as backend issues, e.g. a
 	// connection issue.
-	SetServers([]Server)
+	SetServers(context.Context, []Server)
 }

 // Service is a complete service that's capable of multiple sessions. It has to
@ -837,11 +835,11 @@ type TypingContainer interface {
 	// of typers. This function is usually not needed, as the client will take care
 	// of removing them after TypingTimeout has been reached or other conditions
 	// listed in ServerMessageTypingIndicator are met.
-	RemoveTyper(authorID ID)
+	RemoveTyper(ctx context.Context, authorID ID)
 	// AddTyper appends the typer (author) into the frontend's list of typers, or it
 	// pushes this typer on top of others. The frontend should assume current time
 	// every time AddTyper is called.
-	AddTyper(User)
+	AddTyper(context.Context, User)
 }

 // TypingIndicator optionally extends ServerMessage to provide bidirectional
@ -887,7 +885,7 @@ type TypingIndicator interface {
 type UnreadContainer interface {
 	// SetUnread sets the container's unread state to the given boolean. The
 	// frontend may choose how to represent this.
-	SetUnread(unread bool, mentioned bool)
+	SetUnread(ctx context.Context, unread bool, mentioned bool)
 }

 // UnreadIndicator adds an unread state API for frontends to use. The unread
@ -910,7 +908,7 @@ type UnreadIndicator interface {
 	// the frontend has no use in knowing the error. As such, marking messages as
 	// read is best-effort. The backend is in charge of synchronizing the read state
 	// with the server and coordinating it with reasonable rate limits, if needed.
-	MarkRead(messageID ID)
+	MarkRead(ctx context.Context, messageID ID)
 }

 // User is the interface for an identifiable author. The interface defines that
--- a/cmd/internal/cchat-generator/generate_interface.go
+++ b/cmd/internal/cchat-generator/generate_interface.go
@ -66,6 +66,10 @@ func generateInterfaces(ifaces []repository.Interface) jen.Code {
 					stmt.Params(generateFuncParams(method.Returns, method.ErrorType)...)
 				case repository.SetterMethod:
 					stmt.Params(generateFuncParams(method.Parameters, "")...)
+					stmt.Params(generateFuncParamsErr(repository.NamedType{}, method.ErrorType)...)
+				case repository.ContainerUpdaterMethod:
+					stmt.Params(generateFuncParamsCtx(method.Parameters, "")...)
+					stmt.Params(generateFuncParamsErr(repository.NamedType{}, method.ErrorType)...)
 				case repository.IOMethod:
 					stmt.Params(generateFuncParamsCtx(method.Parameters, "")...)
 					stmt.Params(generateFuncParamsErr(method.ReturnValue, method.ErrorType)...)
--- a/repository/gob/repository.gob
+++ b/repository/gob/repository.gob
--- a/repository/interface.go
+++ b/repository/interface.go
@ -10,6 +10,7 @@ func init() {
 	gob.Register(AsserterMethod{})
 	gob.Register(GetterMethod{})
 	gob.Register(SetterMethod{})
+	gob.Register(ContainerUpdaterMethod{})
 	gob.Register(IOMethod{})
 }

@ -67,17 +68,33 @@ func (m GetterMethod) ReturnError() bool {
 }

 // SetterMethod is a method that sets values. These methods must not do IO, and
-// they have to be non-blocking. They're used only for containers. Actual setter
-// methods implemented by the backend belongs to IOMethods.
-//
-// Clients should always keep track of the values given to setters and free them
-// once the setters are called again with new values.
+// they have to be non-blocking.
 type SetterMethod struct {
 	method

 	// Parameters is the list of parameters in the function. These parameters
 	// should be the parameters to set.
 	Parameters []NamedType
+	// ErrorType is non-empty if the function returns an error at the end of
+	// returns. An error may be returned from the backend if the input is
+	// invalid, but it must not do IO. Frontend setters must never error.
+	ErrorType string
+}
+
+// ContainerUpdaterMethod is a SetterMethod that passes to the container the
+// current context to prevent race conditions when synchronizing.
+// The rule of thumb is that any setter method done inside a method with a
+// context is usually this type of method.
+type ContainerUpdaterMethod struct {
+	method
+
+	// Parameters is the list of parameters in the function. These parameters
+	// should be the parameters to set.
+	Parameters []NamedType
+	// ErrorType is non-empty if the function returns an error at the end of
+	// returns. An error may be returned from the backend if the input is
+	// invalid, but it must not do IO. Frontend setters must never error.
+	ErrorType string
 }

 // IOMethod is a regular method that can do IO and thus is blocking. These
--- a/repository/main.go
+++ b/repository/main.go
@ -817,18 +817,15 @@ var Main = Packages{
 		}, {
 			Comment: Comment{`
 				Configurator is an interface which the backend can implement for a
-				primitive configuration API. Since these methods do return an error,
-				they are allowed to do IO. The frontend should handle this
-				appropriately, including running them asynchronously.
+				primitive configuration API.
 			`},
 			Name: "Configurator",
 			Methods: []Method{
-				IOMethod{
-					method:      method{Name: "Configuration"},
-					ReturnValue: NamedType{Type: "map[string]string"},
-					ErrorType:   "error",
+				GetterMethod{
+					method:  method{Name: "Configuration"},
+					Returns: []NamedType{{Type: "map[string]string"}},
 				},
-				IOMethod{
+				SetterMethod{
 					method:     method{Name: "SetConfiguration"},
 					Parameters: []NamedType{{Type: "map[string]string"}},
 					ErrorType:  "error",
@ -1314,7 +1311,7 @@ var Main = Packages{
 			`},
 			Name: "UnreadIndicator",
 			Methods: []Method{
-				SetterMethod{
+				ContainerUpdaterMethod{
 					method: method{
 						Comment: Comment{`
 							MarkRead marks a message in the server messenger as
@ -1455,7 +1452,7 @@ var Main = Packages{
 			`},
 			Name: "ServersContainer",
 			Methods: []Method{
-				SetterMethod{
+				ContainerUpdaterMethod{
 					method: method{
 						Comment: Comment{`
 							SetServer is called by the backend service to
@ -1476,7 +1473,7 @@ var Main = Packages{
 					},
 					Parameters: []NamedType{{Type: "[]Server"}},
 				},
-				SetterMethod{
+				ContainerUpdaterMethod{
 					method:     method{Name: "UpdateServer"},
 					Parameters: []NamedType{{Type: "ServerUpdate"}},
 				},
@ -1536,7 +1533,7 @@ var Main = Packages{
 			`},
 			Name: "MessagesContainer",
 			Methods: []Method{
-				SetterMethod{
+				ContainerUpdaterMethod{
 					method: method{
 						Comment: Comment{`
 							CreateMessage inserts a message into the container.
@ -1547,11 +1544,11 @@ var Main = Packages{
 					},
 					Parameters: []NamedType{{Type: "MessageCreate"}},
 				},
-				SetterMethod{
+				ContainerUpdaterMethod{
 					method:     method{Name: "UpdateMessage"},
 					Parameters: []NamedType{{Type: "MessageUpdate"}},
 				},
-				SetterMethod{
+				ContainerUpdaterMethod{
 					method:     method{Name: "DeleteMessage"},
 					Parameters: []NamedType{{Type: "MessageDelete"}},
 				},
@ -1640,7 +1637,7 @@ var Main = Packages{
 			`},
 			Name: "LabelContainer",
 			Methods: []Method{
-				SetterMethod{
+				ContainerUpdaterMethod{
 					method: method{Name: "SetLabel"},
 					Parameters: []NamedType{{
 						Type: MakeQual("text", "Rich"),
@ -1656,7 +1653,7 @@ var Main = Packages{
 			`},
 			Name: "ReadContainer",
 			Methods: []Method{
-				SetterMethod{
+				ContainerUpdaterMethod{
 					method: method{
 						Comment: Comment{`
 							AddIndications adds a map of users/authors to the
@ -1667,7 +1664,7 @@ var Main = Packages{
 					},
 					Parameters: []NamedType{{"", "[]ReadIndication"}},
 				},
-				SetterMethod{
+				ContainerUpdaterMethod{
 					method: method{
 						Comment: Comment{`
 							DeleteIndications deletes a list of unused
@ -1701,7 +1698,7 @@ var Main = Packages{
 			`},
 			Name: "UnreadContainer",
 			Methods: []Method{
-				SetterMethod{
+				ContainerUpdaterMethod{
 					method: method{
 						Comment: Comment{`
 							SetUnread sets the container's unread state to the
@ -1727,7 +1724,7 @@ var Main = Packages{
 			`},
 			Name: "TypingContainer",
 			Methods: []Method{
-				SetterMethod{
+				ContainerUpdaterMethod{
 					method: method{
 						Comment: Comment{`
 							AddTyper appends the typer (author) into the
@ -1739,7 +1736,7 @@ var Main = Packages{
 					},
 					Parameters: []NamedType{{"", "User"}},
 				},
-				SetterMethod{
+				ContainerUpdaterMethod{
 					method: method{
 						Comment: Comment{`
 							RemoveTyper explicitly removes the typer with the
@ -1779,7 +1776,7 @@ var Main = Packages{
 			`},
 			Name: "MemberListContainer",
 			Methods: []Method{
-				SetterMethod{
+				ContainerUpdaterMethod{
 					method: method{
 						Comment: Comment{`
 							SetSections (re)sets the list of sections to be the
@ -1795,7 +1792,7 @@ var Main = Packages{
 						{Name: "sections", Type: "[]MemberSection"},
 					},
 				},
-				SetterMethod{
+				ContainerUpdaterMethod{
 					method: method{
 						Comment: Comment{`
 							SetMember adds or updates (or upsert) a member into
@ -1818,7 +1815,7 @@ var Main = Packages{
 						{"member", "ListMember"},
 					},
 				},
-				SetterMethod{
+				ContainerUpdaterMethod{
 					method: method{
 						Comment: Comment{`
 							RemoveMember removes a member from a section. If
Author	SHA1	Message	Date
diamondburned	b5bb0c9bb9	Revert "Replace stop callbacks with contexts" This reverts commit `410ac73469`. The rationale is that the frontend can wrap its own components with a disposable thread-safe wrapper that doesn't work once it's invalidated if it wants the guarantee that the component doesn't work anymore once the context is stopped.	2021-05-04 16:19:13 -07:00
diamondburned	8bfabf58ec	Make SetterMethods ContainerUpdaterMethods This commit separated SetterMethods that are specifically for updating containers to another method type, named ContainerUpdaterMethod. This change is done to force a context parameter into container setters, allowing the frontend to know if an incoming update is valid or not, based on the state of the context given. The validity check should be the same as any other context: select { case <-ctx.Done(): return default: addEvent() } It is crucial, however, to do the checking and updating in the same thread or lock as the context is cancelled. This explicit synchronization is required to prevent any race condition whatsoever with cancellation of the context. The backend must pass in the right context, that is, any context that inherits the cancellation from the frontend. Passing in the invalid context is undefined behavior and will eventually cause a data race.	2021-05-01 21:41:39 -07:00
diamondburned	410ac73469	Replace stop callbacks with contexts This commit removes all stop callbacks in ContainerMethods. The intention is to have backends disconnect callbacks when the context is cancelled, rather than when the stop function is called. This helps get rid of countless race condition flaws caused by the duration between the context being cancelled on one thread and the stop callback being set in another, causing the handlers to not disconnect.	2021-05-01 17:49:25 -07:00
diamondburned	4e11444f6c	Configurator to be SetterMethods This commit changes Configurator's methods to be SetterMethods instead of IOMethods, as Configurator is specifically made for frontend-managed settings just for the backend, so no storing/loading is needed on the backend's side. This commit also changes SetterMethod to allow methods done to the backend to error out, in case the setting value is invalid somehow. Setter methods that are called by the backend (as opposed to the frontend) must never error.	2021-05-01 17:22:01 -07:00