ants icon indicating copy to clipboard operation
ants copied to clipboard

allow NewPoolWithFunc can invoke with nil argument

Open bingoohuang opened this issue 3 years ago β€’ 9 comments

  1. abstract a Task interface for SubmitTask which is more convenient

name: Pull request about: Propose changes to the code title: 'allow NewPoolWithFunc can invoke with nil argument' labels: '' assignees: ''

1. Are you opening this pull request for bug-fixs, optimizations or new feature?

optimizations

2. Please describe how these code changes achieve your intention.

  1. allow NewPoolWithFunc can invoke with nil argument
  2. abstract a Task interface for SubmitTask which is more convenient

3. Please link to the relevant issues (if any).

none

4. Which documentation changes (if any) need to be made/updated because of this PR?

none

4. Checklist

  • [x] I have squashed all insignificant commits.
  • [ ] I have commented my code for explaining package types, values, functions, and non-obvious lines.
  • [x] I have written unit tests and verified that all tests passes (if needed).
  • [ ] I have documented feature info on the README (only when this PR is adding a new feature).
  • [ ] (optional) I am willing to help maintain this change if there are issues with it later.

bingoohuang avatar Jun 15 '21 00:06 bingoohuang

This PR could introduce a problem.

// The address of two struct{} values may be the same.

var a, b struct{}
fmt.Println(&a == &b) // true

My case

package main

import (
	"fmt"
)

var stopArg interface{} = &struct{}{}
var stopArg1 interface{} = &struct{}{}
var stopArg2 interface{} = &struct{}{}

func main() {
	check(stopArg1)
	check(stopArg2)
}

func check(iface interface{}) {
	if iface == stopArg {
		fmt.Println("RETURN")
		return
	}
	fmt.Println("RUN")
}

OUTPUT

RETURN
RETURN

[Process exited 0]

I'm sorry that I offended you. That wasn't my intent.

ioworker0 avatar Jun 15 '21 05:06 ioworker0

@bingoohuang

ioworker0 avatar Jun 15 '21 06:06 ioworker0

@Mutated1994 thanks for your pointing out the potential bug for the stopArg definition

I fixed to var stopArg interface{} = &struct{ a byte }{} and the test cases like

func TestAntsPoolWithFuncNilParam(t *testing.T) {
	var old1 interface{} = &struct{ a byte }{}
	var old2 interface{} = &struct{ a byte }{}
	assert.True(t, old1 != old2)

	isStopArg := func(v interface{}) bool { return v == stopArg }

	var a1 interface{} = &struct{ a byte }{}
	var a2 interface{} = &struct{ a byte }{}
	var a3 interface{} = &struct{ a byte }{}
	assert.False(t, a1 == a2)
	assert.False(t, isStopArg(a1))
	assert.False(t, isStopArg(a2))
	assert.False(t, isStopArg(a3))
	assert.True(t, isStopArg(stopArg))
}

please have a code view for the last change, thanks very much.

bingoohuang avatar Jun 15 '21 14:06 bingoohuang

@Mutated1994 thanks for your pointing out the potential bug for the stopArg definition

I fixed to var stopArg interface{} = &struct{ a byte }{} and the test cases like

func TestAntsPoolWithFuncNilParam(t *testing.T) {
	var old1 interface{} = &struct{ a byte }{}
	var old2 interface{} = &struct{ a byte }{}
	assert.True(t, old1 != old2)

	isStopArg := func(v interface{}) bool { return v == stopArg }

	var a1 interface{} = &struct{ a byte }{}
	var a2 interface{} = &struct{ a byte }{}
	var a3 interface{} = &struct{ a byte }{}
	assert.False(t, a1 == a2)
	assert.False(t, isStopArg(a1))
	assert.False(t, isStopArg(a2))
	assert.False(t, isStopArg(a3))
	assert.True(t, isStopArg(stopArg))
}

please have a code view for the last change, thanks very much.

Dude, it's my pleasure.

LGTM

ioworker0 avatar Jun 17 '21 00:06 ioworker0

Why does ants need this?

panjf2000 avatar Jun 19 '21 11:06 panjf2000

manbe you can ask, why does not ants support this.

There is no need for ants to limit users to pass a nil argument to their register function as some special meaning.

bingoohuang avatar Jun 19 '21 11:06 bingoohuang

Just because the fact that we can do, doesn't mean we should do.

I'm afraid you'll have to prove that this PR is necessary for users, BTW, I also believe there are ways to achieve the same effect based on the existing APIs.

panjf2000 avatar Jun 20 '21 16:06 panjf2000

Just because the fact that we can do, doesn't mean we should do.

I'm afraid you'll have to prove that this PR is necessary for users, BTW, I also believe there are ways to achieve the same effect based on the existing APIs.

TBH, I couldn't agree more.

ioworker0 avatar Jun 20 '21 23:06 ioworker0

Just because the fact that we can do, doesn't mean we should do.

I'm afraid you'll have to prove that this PR is necessary for users, BTW, I also believe there are ways to achieve the same effect based on the existing APIs.

Yeah, I agree with you, but it's the user's choice to make a decision to do or not to do, not the common library or the language itself.

TLDR, It's a user's duty, not the ants'.

bingoohuang avatar Jun 21 '21 00:06 bingoohuang