Skip to content

Enforce timeouts and add simple http2 client test cases #27

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

Closed

Conversation

TysonAndre
Copy link

@TysonAndre TysonAndre commented Oct 1, 2020

Enforce a 10 second timeout - if any request takes longer than that,
then assume that APNs is having networking or server issues so that
the node-apn client can disconnect and reconnect in the background.

Enforce a 10 second timeout - If a request is sent but there have been no successful response to any
request made on that connection in the last 10 seconds, then assume the connection is dead and terminate the connection.

There's probably a better way to do this, e.g. also check if there's been a successful response in the last 10 seconds
for a different request, but this is a starting point and a work in progress.

For #26
Fixes #24

The timeout can be passed in as follows from the Provider to the Client: new apn.Provider({..., timeout: 30000}); // raise the request timeout from 10 seconds to 30 seconds

Node 14 will become an LTS release in October.
@TysonAndre TysonAndre changed the title [WIP] Enforce timeouts and add simple http2 client test cases Enforce timeouts and add simple http2 client test cases Oct 1, 2020
Enforce a 10 second timeout - if any request takes longer than that,
then assume that APNs is having networking or server issues so that
the node-apn client can disconnect and reconnect in the background.

Add tests of goaway, timeouts, protocol errors,
connection termination by the server,
and a few http response codes.

Make eslintConfig work with modern eslint versions

Arrow functions are newer than es6
@davimacedo
Copy link
Member

@funkenstrahlen thoughts here?

Copy link
Member

@davimacedo davimacedo left a comment

Choose a reason for hiding this comment

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

It looks good for me. Let's just wait if @funkenstrahlen has any feedback and we can merge.

Don't time out immediately on the slowest request.

- That would just lead to constantly creating
  and tearing down connections to APNs and possibly getting blocked.

Instead, check if the last successful response was received longer ago
than the configured timeout.
@TysonAndre
Copy link
Author

There's probably a better way to do this, e.g. also check if there's been a successful response in the last 10 seconds
for a different request, but this is a starting point and a work in progress.

I got around to implementing that and added a test. @davimacedo

If, for example, someone were sending thousands of push requests to APNs over dialup quality internet, this PR would probably lead to worse throughput without the latest commit due to constantly destroying and creating connections that were working but limited by reasons other than APNs having silently disconnected

@TysonAndre TysonAndre requested a review from davimacedo October 2, 2020 15:35
@TysonAndre
Copy link
Author

I don't have any other pending changes at the moment unless I see unexpected issues during real usage

@TysonAndre
Copy link
Author

I've been running into issues with http2 and aborting requests due to goaway frames, possibly related to multiple goaway frames being sent at once, which may be something that may or may not only occur in these unit tests with the test http server I've implemented, not in APNs.

  • I'm not sure if the crash is the same place where the preconditions started failling to hold - I'm not experienced with debugging node
  • I'm not sure if this is somehow related to creating a server and tearing it down on the same port, or the http:// insecure http2
  • Dropping node 8 may make sense if this is a concern.
  • I've seen different crashes in Node v12.18.3 before refactoring this

This time, it was reproduced in travis, but only for node 8 so far, https://travis-ci.com/github/parse-community/node-apn/jobs/394316640

https://nodejs.org/en/about/releases/ indicates that node 10 is the oldest version that's still supported, so this

/home/travis/.nvm/versions/node/v8.17.0/bin/node[5214]: ../src/base-object-inl.h:46:virtual node::BaseObject::~BaseObject(): Assertion `persistent_handle_.IsEmpty()' failed.
 1: node::Abort() [/home/travis/.nvm/versions/node/v8.17.0/bin/node]
 2: 0x8cd87b [/home/travis/.nvm/versions/node/v8.17.0/bin/node]
 3: 0x8a8b93 [/home/travis/.nvm/versions/node/v8.17.0/bin/node]
 4: node::http2::Http2Stream::~Http2Stream() [/home/travis/.nvm/versions/node/v8.17.0/bin/node]
 5: node::Environment::RunAndClearNativeImmediates() [/home/travis/.nvm/versions/node/v8.17.0/bin/node]
 6: node::Environment::CheckImmediate(uv_check_s*) [/home/travis/.nvm/versions/node/v8.17.0/bin/node]
 7: 0x9c1a2c [/home/travis/.nvm/versions/node/v8.17.0/bin/node]
 8: uv_run [/home/travis/.nvm/versions/node/v8.17.0/bin/node]
 9: node::Start(uv_loop_s*, int, char const* const*, int, char const* const*) [/home/travis/.nvm/versions/node/v8.17.0/bin/node]
10: node::Start(int, char**) [/home/travis/.nvm/versions/node/v8.17.0/bin/node]
11: __libc_start_main [/lib/x86_64-linux-gnu/libc.so.6]
12: 0x89f601 [/home/travis/.nvm/versions/node/v8.17.0/bin/node]
Aborted (core dumped)

@davimacedo
Copy link
Member

I've inspired in your PR and created a new one: #29

@TysonAndre
Copy link
Author

Closing in favor of #29

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.

send() can hang due to http2 client.request() apparently having no timeout
2 participants