Refactor Backfill

[cnkarz]

Removes channel closing by sender to allow multithreading

[hugjobk]

If you dont close the channel, there is no way for other goroutine to know when to stop reading from that channel.

I suggest you create a channel inside the backfill function and return the read-only channel to the caller. This will give backfill function fully control over the channel preventing other goroutines from closing it.

[hugjobk]

Why dont you make Backfill function non-blocking (start a goroutine inside the function); create the channel inside the function and return it the the caller?

When the caller creates a channel and pass it to a function, it is expected to take ownership over that channel so that the function shouldnt close that channel but leave it for the caller to control the life circle of that channel.

This is how the Backfill function should be used:

c, err := Backfill()
if err != nil {
    return
}
for m := range c {
}

[cnkarz]

Why dont you make Backfill function non-blocking (start a goroutine inside the function)

That is bad practice. It can cause memory leak.

create the channel inside the function and return it the the caller?

Channel would close when function returns.

When the caller creates a channel and pass it to a function, it is expected to take ownership over that channel so that the function shouldnt close that channel but leave it for the caller to control the life circle of that channel.

Sender function can close to signal there is nothing else to send. Otherwise how could you iterate over a channel? It is receiver function that shouldn't close.

[hugjobk]

Why dont you make Backfill function non-blocking (start a goroutine inside the function)

That is bad practice. It can cause memory leak.

create the channel inside the function and return it the the caller?

Channel would close when function returns.

When the caller creates a channel and pass it to a function, it is expected to take ownership over that channel so that the function shouldnt close that channel but leave it for the caller to control the life circle of that channel.

Sender function can close to signal there is nothing else to send. Otherwise how could you iterate over a channel? It is receiver function that shouldn't close.

I dont believe it is a bad practice. This is an example of how a function should be designed in async-await model:

func Backfill() chan<- int {
	c := make(chan int, 1000)
	go func() {
		// Long running process
		for i := 0; i < 1000000; i++ {
			c <- i
		}
		close(c)
	}()
	return c
}

With this design, the caller doesnt need to create a channel nor start a goroutine by itself. It makes an async function act like a sync function from the caller's perspective, which is much simpler to use

[cnkarz]

I thought you wanted to put the whole function inside a separate goroutine which is bad practice, such as:

func Backfill {
    go func() {
      ...
    }()
}

This design will save the caller 1 line of code. So it is more of a convenience. Anyway, I implemented it for you.

old:

func connectorDoingBackfill() {
	c := make(chan types.Log, 1000)
	go ethereum.Backfill(c,...)
	for m := range c{
           ...
        }
}

new:

func connectorDoingBackfill() {
	c := ethereum.Backfill()
	for m := range c{
          .... 
       }
}

[hugjobk]

Why dont you just modify Backfill and BackfillFrom instead of adding a new function? It is confusing for users having too many options to choose from. This will make Backfill and BackfillFrom deprecated btw

[cnkarz]

You can't just modify Backfill and BackfillFrom as they work differently. I created new functions and deprecated these two.

[hugjobk]

Can you return a read-only channel instead? Like this:

chan<- types.Log

[hugjobk]

Return a nil channel can cause unexpexted result for caller who reads from this channel.
You can return a closed channel or return a nil channel with an error instead.

[hugjobk]

Return a nil channel can cause unexpexted result for caller who reads from this channel.
You can return a closed channel or return a nil channel with an error instead

[hugjobk]

Return a nil channel can cause unexpexted result for caller who reads from this channel.
You can return a closed channel or return a nil channel with an error instead

[hugjobk]

Return a nil channel can cause unexpexted result for caller who reads from this channel.
You can return a closed channel or return a nil channel with an error instead

[hugjobk]

Can you return a read-only channel instead? Like this:

chan<- types.Log

[cnkarz]

that's send-only but I changed them to receive-only <-chan ..

[hugjobk]

Approved but here is some things you might want to change:

• If you already close the channel before returning it, you dont need to return an error anymore since it is fine reading from a closed channel.
• If you want to return an error, which I think is not necessary, then you can return a nil channel since returning an error already means you should not read from the channel.

So you can just remove the second return, which is the error, for convinient. Remember to log the error if you dont return them