Refactor Service to actor pattern

[poszu]

With an actor pattern, all state of Service is mutated from a single thread. The select in Service::loop is a synchronization point.

In order to prevent accidental access to the state of Service from the public methods that can be called on other goroutines, a ServiceClient was factored out. It can talk with Service via a channel, sending func(*Service) to access/mutate Service safely. The sent functions are executed serially, which prevents race conditions and should be kept short and must not block.

No goroutine shares a reference to Service hence there is no need for locking at all.

Closes #69
Closes #140
Closes #170

[countvonzero]

can get rid of the mutex using sync.Once?

[poszu]

Probably yes. It would be explicit that Service cannot be restarted after Shutdown(). I will explore this idea in a separate PR as it is not really related to the scope of this one, OK?

[countvonzero]

sure. up to you. it's used often in go-spacemesh.

It would be explicit that Service cannot be restarted after Shutdown()

i don't see there is any way to restart it after the Shutdown() RPC call. is there?

[poszu]

No, there isn't even a shutdown RPC. Shutdown() is called on program exit.

[poszu]

Probably yes. It would be explicit that Service cannot be restarted after Shutdown(). I will explore this idea in a separate PR as it is not really related to the scope of this one, OK?

[fasmat]

I'm still going through the code and trying to understand it so more comments might follow 🙂

[fasmat]

Please add some documentation to Command, e.g. that the command must terminate in a timely manner and should not do file I/O.

Imo a command should also receive the context of the function that actually executes it, so it can return early if the work it does isn't required anymore.

[poszu]

Do you mean the context of the function that creates and schedules the Command or the context of Service::loop? The former is already available as a variable captured by a closure. If I pass the context of Service::loop there will be two contexts available which will be confusing.

[fasmat]

The context of the caller of Command, not the creator of it.

type Command func(ctx context.Context, *Service)

// creator
cmdChan <- func(ctx context.Context, s *Service) {
	select {
	case <- ctx.Done():
		return
	default:
	}

	s.variable = newValue
}

// caller (b.loop)
cmd := <-cmdChan
cmd(ctx, s) // ctx here is the context that cancels loop

[poszu]

It will be confusing because it will look like this:

func (s *ServiceClient) Info(ctx context.Context) (*InfoResponse, error) {
	s.command <- func(serviceCtx context.Context s *Service) {
		// THERE ARE 2 Contexts here, it's confusing
		select {
		case <-ctx.Done():
			return // Info's context is cancelled - GRPC is cancelled
		case <- serviceCtx.Done():
			return // Service's context is cancelled - shutting down
		default:
		}
		s.variable = newValue
	}
}

Relying on the requirement that Command is supposed to be short, I don't see a need to select on any of these. It should just assign newValue / run and return.

[fasmat]

The code you posted is wrong. A Command should never check the context of the function that created it. That doesn't make any logical sense.

With the same argument I could say that a command shouldn't have a Service parameter, because it already has access to the service from its parents scope.

That being sad: I think ServiceClient isn't required as a type. imo inlining it into Service makes the code much more straight forward than having an embedded service client.

[countvonzero]

reading service.go after your change, the differentiation between Service and ServiceClient is still unclear to me.
i don't understand why ServiceClient cannot be inlined.
it seems the public APIs Info/Submit/SetBroadcaster/SetChallengeVerifier all can submit through the command channel.

[poszu]

ServiceClient could be inlined into Service. Not inlining it prevents a whole class of bugs as there is literally no way to access the Service state from ServiceClient in any other way than safely via the commands channel. Thanks to this, it is impossible to accidentally create a data race from the Submit and Info methods.

[countvonzero]

i agree with the general approach to move it to the actor pattern.

tho as is, Service still requires a lock that protects it from doing multiple Start() calls, where members of Service is mutated. more over, there is a ServiceClient that separates out data that need changed during the lifetime of this server.

Service and ServiceClient is not distinguishable from the user of this service. it serves the same purpose and have the same life-span.

i think the cleanest way to do this

• just one struct Service (get rid of ServiceClient)
• make Start() and SetChallengeVerifier() also submit the actual work to commands channel.
• remove all mutex (as the only thing that will be mutated is the commands channel)

[countvonzero]

need to start the loop when service is created too

[poszu]

Thanks, @countvonzero. I'm going to move the start of the event loop to where the Service is created, which will allow me to remove the last mutex. I'm going to do this in a PR removing broadcasting as this is the only reason for delaying the Service start (it cannot operate w/o a broadcaster).

[countvonzero]

approving as there are more upcoming work for the poet spam attack.

giving the author the choice of addressing feedback in this PR, or leave TODO/FIXME now and address them in the upcoming PRs.