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

minor channel refactorings #67

Closed
3 tasks done
plastikfan opened this issue Sep 1, 2023 · 1 comment · Fixed by #69
Closed
3 tasks done

minor channel refactorings #67

plastikfan opened this issue Sep 1, 2023 · 1 comment · Fixed by #69
Assignees
Labels
refactor Refactor code

Comments

@plastikfan
Copy link
Contributor

plastikfan commented Sep 1, 2023

  • close on incorrect jobs channel

I noticed a violation of the private rule in the worker pool. The private rule says that any member of the private property is for the sole use of the associated go routine.

The worker pool is created as:

	wp := &WorkerPool[I, O]{
		private: privateWpInfo[I, O]{
			pool:          make(workersCollection[I, O], noWorkers),
			workersJobsCh: make(chan Job[I], noWorkers),
			finishedCh:    make(FinishedStream, noWorkers),
			cancelCh:      params.CancelCh,
		},
		exec:           params.Exec,
		RoutineName:    GoRoutineName("🧊 worker pool"),
		noWorkers:      noWorkers,
		SourceJobsChIn: params.JobsCh,

		WaitAQ: params.WaitAQ,
	}

so any member of private can only be accessed by the worker pool go routine (pool-gr). Any method executed by the pool-gr can freely access anything inside private, no other go routing can, so it doesn't have to be synchronised. When we talk about channels, the channel members are duplex channels. They can be passed to other methods that expect either a read or write channel.

What I noticed was the run method closes the jobs channel:

close(p.private.workersJobsCh)

This is incorrect, because even though no other gr accesses the run method, there is another reference to the jobs channel: forwardChOut, which should be used instead. So although this does not make any functional difference, we have established a rule for the sake of clarity and we should not be violating it. So the close statement should be

close(forwardChOut)

  • use channel type def

from the private definition:

workersJobsCh: make(chan Job[I], noWorkers),

should use the type definition of the channel instead, ie JobStream:

workersJobsCh: make(JobStream[I], noWorkers),

similarly NewWorkerPoolParams:

JobsCh chan Job[I]

should be

JobsCh JobStream[I]

So again, this will make no functional difference, but we should aim for consistency as much as possible, considering the difficulty in concurrent programming.

  • missing finished channel closure by worker

The current deferred functionality illustrated below:

func (w *worker[I, O]) run(ctx context.Context) {
	defer func() {
		w.finishedChOut <- w.id // ⚠️ non-pre-emptive send, but this should be ok
		fmt.Printf("	<--- 🚀 worker.run(%v) (SENT FINISHED). 🚀🚀🚀\n", w.id)
	}()

What we can see here is that the finished notification is sent to the pool via the finishedChOut channel, but it is not explicitly closed. This probably has no implications, because no consequences have been observed, but as a matter of good practice, we should just close the channel as one would expect. The reason why we don't see any consequences is because there is only ever a single read from this channel from the pool, since there is a separate channel for each worker and once it has been read from, the worker is considered finished and is subsequently discarded.

@plastikfan plastikfan added the refactor Refactor code label Sep 1, 2023
@plastikfan plastikfan self-assigned this Sep 1, 2023
@plastikfan plastikfan changed the title minor refactorings minor channel refactorings Sep 1, 2023
@plastikfan
Copy link
Contributor Author

Actually, if we try to close the finished channel, we see a timeout occur, so this aspect will not be implemented.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
refactor Refactor code
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant