request with context #57

xjewer · 2016-11-29T14:22:30Z

With context you can to cancel request when it goes longer than necessary https://godoc.org/context#WithTimeout

import "context" works only >= go1.7, so i divided PushWithCtx method along with build tags !go1.7 and go1.7 into the distinct files with "net/context" and "context" respectively

coveralls · 2016-11-29T14:24:28Z

Coverage increased (+0.2%) to 89.822% when pulling c2d3389 on xjewer:feature_request_with_context into 734432d on sideshow:master.

coveralls · 2016-11-29T14:36:38Z

Coverage increased (+0.2%) to 89.822% when pulling 3176f9f on xjewer:feature_request_with_context into 734432d on sideshow:master.

coveralls · 2016-11-29T14:37:56Z

Coverage increased (+0.2%) to 89.822% when pulling 9593f63 on xjewer:feature_request_with_context into 734432d on sideshow:master.

coveralls · 2016-11-30T08:05:26Z

Coverage increased (+0.2%) to 89.822% when pulling b2dabee on xjewer:feature_request_with_context into 734432d on sideshow:master.

…olang version and context package

coveralls · 2016-11-30T08:08:13Z

Coverage increased (+0.9%) to 90.521% when pulling 16b818e on xjewer:feature_request_with_context into 734432d on sideshow:master.

xjewer · 2016-11-30T08:14:23Z

/cc @sideshow

sideshow · 2017-01-04T23:32:22Z

@xjewer this looks really good, Thanks for the PR

nordicdyno · 2017-01-14T12:21:00Z

client_push_go17.go

@@ -0,0 +1,50 @@
+// +build go1.7


I think it's better to use !go1.6 here for Go >=1.8 compatibility. Anyway Go < 1.6 is not supported by this lib

This is okay. "from Go version 1.7 onward" https://golang.org/pkg/go/build/

Though I don't know if it's needed.

xjewer · 2017-02-03T12:52:45Z

@xjewer this looks really good, Thanks for the PR

So, what about this PR? Would you merge it or not? @sideshow

nathany

I think it makes sense to support context. @sideshow do you agree?

Maybe this could be done in a cleaner way though (see below)

nathany · 2017-02-03T18:36:56Z

client_push_pre_go17.go

+// gateway, or an error if something goes wrong.
+//
+// Context with deadline allows to close long request when context timeout exceed. It can be nil, for back compatibility
+func (c *Client) PushWithCtx(n *Notification, ctx context.Context) (*Response, error) {


There should be a way to minimize the duplication between the Go 1.6 and Go 1.7+ versions of this.

context.Context is an interface with the same definition in both packages, so they are interchangeable.

https://golang.org/pkg/context/#Context
https://godoc.org/golang.org/x/net/context#Context

I'm not sure the best way to do this, but even redefining the Context interface locally should allow either one to be passed in.

I am not sure, that it costs the time to try to reduce a duplication in this part of code

The problem with the duplication is that future changes need to made in both places. If they are not, the code could diverge in behaviour between Go versions. If there isn't any other option, good tests and continuous integration on multiple Go versions can help.

I await a verdict/suggestions from @sideshow.

I agree @nathany there we should look to remove the duplication given the fact that Context is part of the standard library now. @xjewer I can help if needed

nathany · 2017-02-03T18:37:39Z

client_test.go

@@ -16,6 +16,7 @@ import (
 	apns "github.com/sideshow/apns2"
 	"github.com/sideshow/apns2/certificate"
 	"github.com/stretchr/testify/assert"
+	"golang.org/x/net/context"


It would be good to see two versions of the context tests in separate files with appropriate build tags, just to ensure that it works with either package.

nathany · 2017-02-03T18:41:28Z

client.go

@@ -93,31 +90,10 @@ func (c *Client) Production() *Client {
 // transparently before sending the notification. It will return a Response
 // indicating whether the notification was accepted or rejected by the APNs
 // gateway, or an error if something goes wrong.
+//
+// It wraps PushWithCtx for back compatibility.
 func (c *Client) Push(n *Notification) (*Response, error) {


@sideshow How important is API stability for this project? Do you want to keep the no-context version around?

I think its pretty important to keep this old version around @nathany just so we don't break other projects that depend on it

coveralls · 2017-02-07T10:17:14Z

Coverage increased (+0.8%) to 90.499% when pulling 2e8e495 on xjewer:feature_request_with_context into 19ab0d4 on sideshow:master.

sideshow

@nathany thanks for looking at this
@xjewer Sorry for taking so long to review, I was on holiday. I think the whole thing looks awesome, would love if we could remove the duplication as @nathany suggested between the Go 1.6 and Go 1.7+ if possible before merging.

sideshow · 2017-02-15T07:10:02Z

client_push_pre_go17.go

+// gateway, or an error if something goes wrong.
+//
+// Context with deadline allows to close long request when context timeout exceed. It can be nil, for back compatibility
+func (c *Client) PushWithCtx(n *Notification, ctx context.Context) (*Response, error) {


I agree @nathany there we should look to remove the duplication given the fact that Context is part of the standard library now. @xjewer I can help if needed

sideshow · 2017-02-15T07:10:50Z

client_push_pre_go17_test.go

+	"golang.org/x/net/context"
+)
+
+func TestClient_PushWithCtx_WithTimeout(t *testing.T) {


Any chance we could change this to TestClientPushWithCtxWithTimeout just for consistency

sideshow · 2017-02-15T07:16:10Z

client.go

@@ -93,31 +90,10 @@ func (c *Client) Production() *Client {
 // transparently before sending the notification. It will return a Response
 // indicating whether the notification was accepted or rejected by the APNs
 // gateway, or an error if something goes wrong.
+//
+// It wraps PushWithCtx for back compatibility.
 func (c *Client) Push(n *Notification) (*Response, error) {


I think its pretty important to keep this old version around @nathany just so we don't break other projects that depend on it

sideshow · 2017-04-13T06:27:19Z

@xjewer Thanks for this PR. @nathany thanks for the review.

I have rebased and changed a few things here #81 and would like your feedback.

I am closing this one but please feel free to weigh in on the new PR.

xjewer added 2 commits November 29, 2016 17:35

add a request context

2ff9521

fix a variable 'certificate' that collides with imported package name

9593f63

xjewer force-pushed the feature_request_with_context branch from 3176f9f to 9593f63 Compare November 29, 2016 14:35

use build tags for different realization the PushWithCtx depends on g…

16b818e

…olang version and context package

xjewer force-pushed the feature_request_with_context branch from b2dabee to 16b818e Compare November 30, 2016 08:06

nordicdyno reviewed Jan 14, 2017

View reviewed changes

nathany suggested changes Feb 3, 2017

View reviewed changes

nathany reviewed Feb 3, 2017

View reviewed changes

separate the push test on different go version

2e8e495

sideshow reviewed Feb 15, 2017

View reviewed changes

sideshow mentioned this pull request Apr 9, 2017

Feature request context #81

Merged

sideshow closed this Apr 13, 2017

xjewer deleted the feature_request_with_context branch April 24, 2017 07:15

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

request with context #57

request with context #57

xjewer commented Nov 29, 2016 •

edited

Loading

coveralls commented Nov 29, 2016 •

edited

Loading

coveralls commented Nov 29, 2016 •

edited

Loading

coveralls commented Nov 29, 2016 •

edited

Loading

coveralls commented Nov 30, 2016 •

edited

Loading

coveralls commented Nov 30, 2016 •

edited

Loading

xjewer commented Nov 30, 2016

sideshow commented Jan 4, 2017

nordicdyno Jan 14, 2017 •

edited

Loading

nathany Jan 16, 2017

xjewer commented Feb 3, 2017

nathany left a comment

nathany Feb 3, 2017

xjewer Feb 7, 2017

nathany Feb 13, 2017

sideshow Feb 15, 2017 •

edited

Loading

nathany Feb 3, 2017

xjewer Feb 7, 2017

nathany Feb 3, 2017

sideshow Feb 15, 2017

coveralls commented Feb 7, 2017 •

edited

Loading

sideshow left a comment

sideshow Feb 15, 2017 •

edited

Loading

sideshow Feb 15, 2017

sideshow Feb 15, 2017

sideshow commented Apr 13, 2017

request with context #57

request with context #57

Conversation

xjewer commented Nov 29, 2016 • edited Loading

coveralls commented Nov 29, 2016 • edited Loading

coveralls commented Nov 29, 2016 • edited Loading

coveralls commented Nov 29, 2016 • edited Loading

coveralls commented Nov 30, 2016 • edited Loading

coveralls commented Nov 30, 2016 • edited Loading

xjewer commented Nov 30, 2016

sideshow commented Jan 4, 2017

nordicdyno Jan 14, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xjewer commented Feb 3, 2017

nathany left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sideshow Feb 15, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Feb 7, 2017 • edited Loading

sideshow left a comment

Choose a reason for hiding this comment

sideshow Feb 15, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sideshow commented Apr 13, 2017

xjewer commented Nov 29, 2016 •

edited

Loading

coveralls commented Nov 29, 2016 •

edited

Loading

coveralls commented Nov 29, 2016 •

edited

Loading

coveralls commented Nov 29, 2016 •

edited

Loading

coveralls commented Nov 30, 2016 •

edited

Loading

coveralls commented Nov 30, 2016 •

edited

Loading

nordicdyno Jan 14, 2017 •

edited

Loading

sideshow Feb 15, 2017 •

edited

Loading

coveralls commented Feb 7, 2017 •

edited

Loading

sideshow Feb 15, 2017 •

edited

Loading