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

Address eventloop's double registerCallback call when using promises.New #3180

Merged
merged 2 commits into from
Jul 10, 2023

Conversation

oleiade
Copy link
Member

@oleiade oleiade commented Jul 10, 2023

What?

This Pull Request addresses the misuse of named arguments introduced by #3176, and fixes the resulting registerCallback called twice issue that followed from it.

It also adds two minimal tests covering ensuring the resolve and reject callbacks are effective. I wasn't entirely sure how to properly test that function; this is the simplest thing I came up with. I'm all ears 👂🏻 for alternatives and suggestions to improve it 🙇🏻

Why?

As I started using the promises.New function in another stream of work, I noticed that as soon as I used the promises.New function, I would end up having registerCallback called twice panic all over. I tracked down the issue, and it turned out that it was because we needed to use Go named return arguments properly.

Checklist

  • I have performed a self-review of my code.
  • I have added tests for my changes.
  • I have run linter locally (make ci-like-lint) and all checks pass.
  • I have run tests locally (make tests) and all tests pass.
  • I have commented on my code, particularly in hard-to-understand areas.

Related PR(s)/Issue(s)

ref #3176
ref #3176 (review)

@oleiade oleiade added the bug label Jul 10, 2023
@oleiade oleiade added this to the v0.46.0 milestone Jul 10, 2023
@oleiade oleiade self-assigned this Jul 10, 2023
@github-actions github-actions bot requested review from codebien and imiric July 10, 2023 10:21
@codecov-commenter
Copy link

Codecov Report

Merging #3180 (5d5e15b) into master (3906dcf) will increase coverage by 0.02%.
The diff coverage is 100.00%.

❗ Current head 5d5e15b differs from pull request most recent head 75c8d47. Consider uploading reports for the commit 75c8d47 to get more accurate results

@@            Coverage Diff             @@
##           master    #3180      +/-   ##
==========================================
+ Coverage   72.77%   72.80%   +0.02%     
==========================================
  Files         255      256       +1     
  Lines       19686    19697      +11     
==========================================
+ Hits        14327    14340      +13     
+ Misses       4463     4462       -1     
+ Partials      896      895       -1     
Flag Coverage Δ
ubuntu 72.73% <100.00%> (+0.02%) ⬆️
windows 72.64% <100.00%> (+0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
js/promises/promises.go 100.00% <100.00%> (ø)

... and 1 file with indirect coverage changes

callback := vu.RegisterCallback()

return p, func(i interface{}) {
callback(func() error {
resolve(i)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So for future reference - the problem here was that resolve here is the same resolve we return - which is the function we in. So this just recurse inside itself.

@oleiade oleiade merged commit 1b446a4 into master Jul 10, 2023
23 checks passed
@oleiade oleiade deleted the fix/promises.New-double-registerCallback branch July 10, 2023 11:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants