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

Upgrade to Go 1.17 #331

Merged
merged 5 commits into from Oct 7, 2021
Merged

Upgrade to Go 1.17 #331

merged 5 commits into from Oct 7, 2021

Conversation

ghost
Copy link

@ghost ghost commented Oct 6, 2021

Upgrade to go 1.17

  • update go.mod
  • update Dockerfile
  • update GitHub actions
  • fix new go vet errors

resolves #330


Reviewer checklist
  • Read PR description: a summary about the changes is required
  • Changelog updated
  • Documentation: docs/{Reference, Cli, ...}, Docker and cli help/usage
  • Pulled branch, manually tested
  • Verified requirements are met
  • Reviewed the code
  • Reviewed the related tests

@ghost ghost changed the title Go 1.17 Upgrade to Go 1.17 Oct 6, 2021
@afflerbach afflerbach requested a review from malud October 6, 2021 11:41
@ghost
Copy link
Author

ghost commented Oct 6, 2021

I could fix the race condition by doing something like this, what do you think about it?
handler/endpoint.go:102

- go e.opts.Proxies.Produce(subCtx, req, proxyResults)
- go e.opts.Requests.Produce(subCtx, req, requestResults)
+ go e.opts.Proxies.Produce(subCtx, req.Clone(context.TODO()), proxyResults)
+ go e.opts.Requests.Produce(subCtx, req.Clone(context.TODO()), requestResults)

@alex-schneider
Copy link
Contributor

@malud, the race condition fix from develerik works for me nicely. but i would use reqCtx from line 71 instead of context.TODO(). what do you mean?

@malud
Copy link
Collaborator

malud commented Oct 6, 2021

This data races seems to be related to #323 .
I would like to fix the root cause, using TODO() is unfortunately not helpful since the required context (and cancels) are not available anymore for usage at backend/roundtrip level.
Additionally the Produce methods will clone the request already, so cloning them to clone them again seems to be not the best approach solving this one.

@develerik thanks for your contribution so far. We will look into this race condition since its unrelated to your changes and will give you asap a ping if we have solved this.

@malud
Copy link
Collaborator

malud commented Oct 7, 2021

thanks @develerik

@malud malud merged commit ff64af6 into coupergateway:master Oct 7, 2021
@ghost ghost deleted the go-1.17 branch October 7, 2021 10:56
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

Successfully merging this pull request may close these issues.

Upgrade to Go 1.17
3 participants