Skip to content

Client should not block on GOMAXPROCS receivers.#574

Merged
n3wscott merged 6 commits intocloudevents:masterfrom
n3wscott:blocking-client-maxprocs
Sep 3, 2020
Merged

Client should not block on GOMAXPROCS receivers.#574
n3wscott merged 6 commits intocloudevents:masterfrom
n3wscott:blocking-client-maxprocs

Conversation

@n3wscott
Copy link
Copy Markdown
Member

@n3wscott n3wscott commented Aug 28, 2020

Fixes: #569

  • Change client to fork off the call to invoker inside the polling loop.
  • adding a test to validate the blocking GOMAXPROCS is an issue in the client

Signed-off-by: Scott Nichols snichols@vmware.com

Signed-off-by: Scott Nichols <snichols@vmware.com>
@n3wscott n3wscott changed the title Client should not block on GOMAXPROC receivers. Client should not block on GOMAXPROCS receivers. Aug 28, 2020
Signed-off-by: Scott Nichols <snichols@vmware.com>
@n3wscott n3wscott marked this pull request as ready for review August 28, 2020 17:45
Scott Nichols added 2 commits August 28, 2020 10:52
Signed-off-by: Scott Nichols <snichols@vmware.com>
Signed-off-by: Scott Nichols <snichols@vmware.com>
@n3wscott
Copy link
Copy Markdown
Member Author

@liu-cong can you try this patch and let me know if it solves your issue?

Scott Nichols added 2 commits August 28, 2020 11:04
Signed-off-by: Scott Nichols <snichols@vmware.com>
…ow with the blocking tests. Bump integration timeout.

Signed-off-by: Scott Nichols <snichols@vmware.com>
@slinkydeveloper slinkydeveloper self-requested a review August 31, 2020 08:41
cecontext.LoggerFrom(ctx).Warnf("Error while handling a message: %s", err)
}
// Do not block on the invoker.
wg.Add(1)
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Do we really need to synchronize the Invoke on the wg? If Invoke ends after these goroutines loop, is this a potential issue?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

it just lets us know to block if invoke is happening when the process is spinning down

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

that's my point, do we need to wait for invoke to finish before spinning down these goroutines? isn't this done automatically by the runtime?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

It is not done automatically. We would return from this loop and exit the process before the invokers had a chance to finish on a soft sig-term

@n3wscott
Copy link
Copy Markdown
Member Author

n3wscott commented Sep 2, 2020

Ping on @liu-cong

@slinkydeveloper want to LGTM?

@slinkydeveloper slinkydeveloper self-requested a review September 2, 2020 15:38
@liu-cong
Copy link
Copy Markdown
Contributor

liu-cong commented Sep 2, 2020

/lgtm Thanks @n3wscott for the quick fix!

I haven't tried this patch. However, I previously tried a similar/same patch locally so I believe it will work as expected :)

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.

CE receiver client can only handle GOMAXPROCS concurrent requests

3 participants