Drop context arg from HTTP handler #58
Conversation
knative-prow
bot
added
the
size/M
| @@ -32,7 +32,7 @@ const ( | |||
| // Start an intance using a new Service | |||
| // Note that for CloudEvent Handlers this effectively accepts ANY because | |||
| // the actual type of the handler function is determined later. | |||
| func Start(f any) error { | |||
| func Start(f Handler) error { | |||
There was a problem hiding this comment.
I changed this - since we want a proper Handler to be passed here.
We shouldn't be failing this during runtime - it will swallow errors.
There was a problem hiding this comment.
Well, I thought so too. But then thought it would be possible one might want a Function which fires "OnStart" and "OnStop" but does not accept HTTP requests, or is set to never scale to 0 and performs some action on a chron-like timer. This would open us up to having Functions which fill very different roles than the expected, request-response HTTP cycle... and I think that's a good thing. WDYT?
There was a problem hiding this comment.
This would open us up to having Functions which fill very different roles than the expected, request-response HTTP cycle
For these use cases I'd use a different template and not the HTTP one
There was a problem hiding this comment.
Secondly, if the user typos Handle then they won't get feedback here because it's loosely typed.
|
/hold for @lkingland |
knative-prow
bot
added
the
do-not-merge/hold
|
/assign @lkingland |
|
/hold cancel |
knative-prow
bot
removed
the
do-not-merge/hold
| @@ -91,7 +91,7 @@ func (s *Service) Start(ctx context.Context) (err error) { | |||
| s.handleSignals() | |||
|
|
|||
| go func() { | |||
| if err = s.Server.Serve(s.listener); err != http.ErrServerClosed { | |||
There was a problem hiding this comment.
data race was here
| @@ -123,7 +120,7 @@ func (s *Service) Start(ctx context.Context) (err error) { | |||
|
|
|||
| // Listen and serve | |||
| go func() { | |||
| if err = s.Server.Serve(s.listener); err != http.ErrServerClosed { | |||
| if err := s.Serve(s.listener); err != http.ErrServerClosed { | |||
There was a problem hiding this comment.
data race was here
| @@ -32,7 +32,7 @@ const ( | |||
| // Start an intance using a new Service | |||
| // Note that for CloudEvent Handlers this effectively accepts ANY because | |||
| // the actual type of the handler function is determined later. | |||
| func Start(f any) error { | |||
| func Start(f Handler) error { | |||
There was a problem hiding this comment.
Well, I thought so too. But then thought it would be possible one might want a Function which fires "OnStart" and "OnStop" but does not accept HTTP requests, or is set to never scale to 0 and performs some action on a chron-like timer. This would open us up to having Functions which fill very different roles than the expected, request-response HTTP cycle... and I think that's a good thing. WDYT?
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: dprotaso, lkingland The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
knative-prow
bot
added
the
approved
No description provided.