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

wrong time to call 'TraceBatchFinishFunc' #63

Open
jimyang112 opened this issue Dec 1, 2019 · 2 comments
Open

wrong time to call 'TraceBatchFinishFunc' #63

jimyang112 opened this issue Dec 1, 2019 · 2 comments

Comments

@jimyang112
Copy link

ctx, finish := b.tracer.TraceBatch(originalContext, keys)
defer finish(items) // this should be moved

func() {
	defer func() {
		if r := recover(); r != nil {
			panicErr = r
			if b.silent {
				return
			}
			const size = 64 << 10
			buf := make([]byte, size)
			buf = buf[:runtime.Stack(buf, false)]
			log.Printf("Dataloader: Panic received in batch function:: %v\n%s", panicErr, buf)
		}
	}()
	items = b.batchFn(ctx, keys)
}()

// defer finish(items) 
@mjm
Copy link

mjm commented Jan 27, 2020

I just noticed this too. By calling it here, it captures the wrong items slice, so the finish function from the tracing always gets an empty slice.

@nicksrandall
Copy link
Member

PRs are open :)

mjq added a commit to mjq/dataloader that referenced this issue Mar 29, 2021
The slice of *Result objects passed to TraceBatchFinishFunc was always
empty. By moving the `defer finish(items)` down below where items is
assigned the results of the batch function, tracers are passed the
actual results.

Fixes graph-gophers#63.
mjq added a commit to mjq/dataloader that referenced this issue Mar 29, 2021
The slice of *Result objects passed to TraceBatchFinishFunc was always
empty. By moving the `defer finish(items)` down below where items is
assigned the results of the batch function, tracers are passed the
actual results.

Fixes graph-gophers#63.
mjq added a commit to mjq/dataloader that referenced this issue Mar 30, 2021
The slice of *Result objects passed to TraceBatchFinishFunc was always
empty. By moving the `defer finish(items)` down below where items is
assigned the results of the batch function, tracers are passed the
actual results.

Fixes graph-gophers#63.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants