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.

Sign up for GitHub

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

Jump to bottom

Add support for invoking with context. #25

Merged
merged 1 commit into from
Aug 9, 2019
Merged

Add support for invoking with context. #25

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 8 additions & 1 deletion types/controller.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
package types

import (
"context"
"fmt"
"log"
"sync"
Expand Down Expand Up @@ -111,7 +112,13 @@ func (c *Controller) Subscribe(subscriber ResponseSubscriber) {
// Invoke attempts to invoke any functions which match the
// topic the incoming message was published on.
func (c *Controller) Invoke(topic string, message *[]byte) {
c.Invoker.Invoke(c.TopicMap, topic, message)
c.InvokeWithContext(context.Background(), topic, message)
}

// InvokeWithContext attempts to invoke any functions which match the topic
// the incoming message was published on while propagating context.
func (c *Controller) InvokeWithContext(ctx context.Context, topic string, message *[]byte) {
c.Invoker.InvokeWithContext(ctx, c.TopicMap, topic, message)
}

// BeginMapBuilder begins to build a map of function->topic by
Expand Down
19 changes: 15 additions & 4 deletions types/invoker.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ package types

import (
"bytes"
"context"
"fmt"
"io"
"io/ioutil"
Expand All @@ -19,6 +20,7 @@ type Invoker struct {
}

type InvokerResponse struct {
Context context.Context
Copy link
Member

Choose a reason for hiding this comment

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

Should the context really be sent back with the Response? How would that be used? Do you envision it behaving like the http.Response object with its own r.Context()? I wonder if that is actually recommended, I think the http.Response object has an embedded context because they didn't want to change the http.Handler interface

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi @LucasRoesler, thank you for reviewing.

Should the context really be sent back with the Response?

I believe so.

How would that be used?

I've provided an example above.

Do you envision it behaving like the http.Response object with its own r.Context()?

I don't think I quite understand what you are referring to, but the way I envision this context to work is as a something that groups together contextual information that can be used to correlate responses with the original request as detailed above.

Copy link
Member

Choose a reason for hiding this comment

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

I have thought more about this and I don't see an alternative approach. Adding the context here seems to make sense to me

Also, I realize that I have a typo in my original comment, I meant http.Request, not http.Response.

Copy link
Member

Choose a reason for hiding this comment

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

A custom string, or a map could be used instead of the context.

My fear is that people will expect the context to be used as something more than the intent Bruno is proposing (as a map/bag).

The context object also has timeout / duration / cancellation / done behaviour, but this behaviour hasn't been implemented in the PR.

Would a map of string[string] be sufficient for what you need Bruno, or a string of some sort?

Copy link
Member

Choose a reason for hiding this comment

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

Lucas has just pointed me at this line -> https://github.com/openfaas-incubator/connector-sdk/pull/25/files#diff-cd0839c41d9c260fb20a0c53fe284e3eR86

If this context is being used for more than just a bag for a request / message ID and is fully functional as a Context, I think Lucas + I are in agreement with moving the change forward with #28 raised to cover my other concerns.

Body *[]byte
Header *http.Header
Status int
Expand All @@ -38,9 +40,15 @@ func NewInvoker(gatewayURL string, client *http.Client, printResponse bool) *Inv

// Invoke triggers a function by accessing the API Gateway
func (i *Invoker) Invoke(topicMap *TopicMap, topic string, message *[]byte) {
i.InvokeWithContext(context.Background(), topicMap, topic, message)
}

// Invoke triggers a function by accessing the API Gateway while propagating context
func (i *Invoker) InvokeWithContext(ctx context.Context, topicMap *TopicMap, topic string, message *[]byte) {
if len(*message) == 0 {
i.Responses <- InvokerResponse{
Error: fmt.Errorf("no message to send"),
Context: ctx,
Error: fmt.Errorf("no message to send"),
}
}

Expand All @@ -51,16 +59,18 @@ func (i *Invoker) Invoke(topicMap *TopicMap, topic string, message *[]byte) {
gwURL := fmt.Sprintf("%s/%s", i.GatewayURL, matchedFunction)
reader := bytes.NewReader(*message)

body, statusCode, header, doErr := invokefunction(i.Client, gwURL, reader)
body, statusCode, header, doErr := invokefunction(ctx, i.Client, gwURL, reader)

if doErr != nil {
i.Responses <- InvokerResponse{
Error: errors.Wrap(doErr, fmt.Sprintf("unable to invoke %s", matchedFunction)),
Context: ctx,
Error: errors.Wrap(doErr, fmt.Sprintf("unable to invoke %s", matchedFunction)),
}
continue
}

i.Responses <- InvokerResponse{
Context: ctx,
Body: body,
Status: statusCode,
Header: header,
Expand All @@ -70,9 +80,10 @@ func (i *Invoker) Invoke(topicMap *TopicMap, topic string, message *[]byte) {
}
}

func invokefunction(c *http.Client, gwURL string, reader io.Reader) (*[]byte, int, *http.Header, error) {
func invokefunction(ctx context.Context, c *http.Client, gwURL string, reader io.Reader) (*[]byte, int, *http.Header, error) {

httpReq, _ := http.NewRequest(http.MethodPost, gwURL, reader)
httpReq.WithContext(ctx)

if httpReq.Body != nil {
defer httpReq.Body.Close()
Expand Down