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

Refactor synchronous interface #33

Closed
wants to merge 22 commits into from
Closed

Conversation

bdotdub
Copy link
Contributor

@bdotdub bdotdub commented Mar 2, 2015


Opened in response to #31 for inline comments

)

type mockConn struct {
Copy link
Contributor

Choose a reason for hiding this comment

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

is mockConn something that users of this library would find useful when testing?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'd say no with the implementation as it stands – it's not a terribly generic solution. I figured since Conn is now an interface, users of the library would write their own

conn: conn,
connm: sync.Mutex{},
idm: sync.Mutex{},
b: newBuffer(50),
Copy link
Contributor

Choose a reason for hiding this comment

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

why 50? :-)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

¯_(ツ)_/¯ haha

Definitely should be able to specify this, but seemed "large enough" for me.

Copy link
Contributor

Choose a reason for hiding this comment

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

😄

Maybe at least extract to a const?

Copy link
Contributor

Choose a reason for hiding this comment

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

There is also a cleanup thread running that constantly checks the oldest notification in the ‘Sent’ queue to see if it’s older than a few seconds and if so, it is assumed to have been successfully sent and is removed from the ‘Sent’ queue. - The Problem With Apples Push Notification Service

It can take a while for the dropped connection to make its way from APNs back to your server just because of normal latency. It's possible to send over 500 notifications before a write fails because of the connection being dropped. Around 1,700 notifications writes can fail just because the pipe is full, so just retry in that case once the stream is ready for writing again. - Push Notification Throughput and Error Checking

return s.id
}

func (s *session) readErrors() {
Copy link
Contributor

Choose a reason for hiding this comment

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

Some details from Apple on error checking:
https://developer.apple.com/library/ios/technotes/tn2265/_index.html#//apple_ref/doc/uid/DTS40010376-CH1-TNTAG44

If your development tool of choice supports multiple threads or interprocess communication, you could have a thread or process waiting for an error response all the time and let the main sending thread or process know when it should give up and retry.

We launch readErrors on a goroutine, but it looks like it will only read once and then end. I assume read is blocking though, but is this enough?

@nathany
Copy link
Contributor

nathany commented Apr 30, 2015

Continuing work on the develop branch.

Comments here have been addressed or mentioned in #42 for later.

@nathany nathany closed this Apr 30, 2015
id uint32
idm sync.Mutex

err SessionError
Copy link
Contributor

Choose a reason for hiding this comment

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

Is storing the last error enough?

Previously it sent all errors.

@nathany nathany mentioned this pull request May 20, 2015
3 tasks
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.

3 participants