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

fix up channel defs #70

Closed
plastikfan opened this issue Sep 3, 2023 · 2 comments · Fixed by #78
Closed

fix up channel defs #70

plastikfan opened this issue Sep 3, 2023 · 2 comments · Fixed by #78
Assignees
Labels
refactor Refactor code

Comments

@plastikfan
Copy link
Contributor

plastikfan commented Sep 3, 2023

I've been having a few problems with the channel type definitions and using them to create channels, particular when you have to pass in the same channel to different entities, but those entities either have a write or read relationship with the channel but not both. An example of this is with the outputs channel.

Here is the problem with the output channe;l

  • pipeline defines outputsCh as chan boost.JobOutput[O]:

outputsCh chan boost.JobOutput[O]

This is our first problem right here. outputsCh is an output channel as far as the worker pool and the workers are concerned. They should only ever have write access, so we want to prevent accidental read. (My bad, I just left it as chan boost.JobOutput[O] to fix compile errors, but it has left a nagging doubt in mind ever since, hence this issue).

The outputs channel is left for he client to create it, so they are in control. In this case, the pipeline represents the client. This propagates all the way out to the test definition:

	DescribeTable("stream of jobs",
		func(ctxSpec SpecContext, entry *poolTE) {
			defer leaktest.Check(GinkgoT())()

			outputCh := lo.TernaryF(entry.outputsChSize > 0,
				func() TestOutputStream {
					return make(TestOutputStream, entry.outputsChSize)
				},
				func() TestOutputStream {
					return nil
				},
			)
			pipe := start[TestInput, TestOutput](outputCh)

The problem here is that the channel outputCh is a duplex channel. We should modify the anon function to return both ends of the same channel. So instead of just returning a single duplex channel, we create the duplex channel internally then return 2 chanels the read end and the write end; note it is not really 2 channels, just 2 different views on the same channel.

We can then pass the appropriate typed channel to the entities that need them. The worker pool needs to write to it, so we pass it the writer end of the channel and the consumer needs to read from it so we pass in the reader end.

Once we fix this, the flow of the correctly typed output channel (that doesn't accidentally have read access) to the rest of the worker pool and workers will be resolved.

@plastikfan plastikfan added the feature New feature or request label Sep 3, 2023
@plastikfan plastikfan self-assigned this Sep 3, 2023
@plastikfan plastikfan added refactor Refactor code and removed feature New feature or request labels Sep 3, 2023
@plastikfan
Copy link
Contributor Author

Actually, I don't think we need to fix this, because:

func StartConsumer[O any](
	parentContext context.Context,
	quitter boost.AnnotatedWgQuitter,
	outputsChIn boost.OutputStreamR[O],
	interval time.Duration,
)

outputsChIn already is read only channel (boost.OutputStreamR[O])

outputsCh chan boost.JobOutput[O] on the pipeline is the duplex definition; that is why it's called outputsCh without an In or Out decoration, so this is where we get our type safety.

@plastikfan
Copy link
Contributor Author

plastikfan commented Sep 25, 2023

Using chan typedefs is looking like more hassle than it's worth. It was just an experiemt to see how useful it is to define channel type defs. In reality, it makes the channel types more complex when it was supposed to simplify. The typedefs do make channel code more maintainable, because the of the channel type defs, but it creates complications because the raw channel type (chan T) is not compatible with the channel type defs; this leads to having to define raw channels in certain situations making the code inconsistent, which is the opposite of what was intented.

I am not sure whether I'll continue with this pattern. I would like to but this experience leads me to think that we should just define channels without typedefs. One useful part of the pattern is the Duplex which should still stand as that is a valid abstraction.

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