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

Allow custom timeout for pull requests #136

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

bl0ggy
Copy link
Contributor

@bl0ggy bl0ggy commented Feb 19, 2020

Add the possibility to choose the timeout of a PullMessagesRequest.
Also add the possibility to choose the timeout of a http request.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.02%) to 87.322% when pulling 787a0ff on bl0ggy:custom_pull_message_timeout into fb421bd on agsh:master.

@RogerHardiman
Copy link
Collaborator

Thanks for the PR.
I have a question.
I'd hard coded the Pull timeout to 5 seconds in my original code as it was less than the default Socket Timeout period on Linux and Windows.

With your change the user of the library could have a 30 second timeout.
So we need to ensure the Socket Timeout is set to the EventTimout plus 5 (or 10) seconds.

I'd expect we can change the socket timeout in Node. Would be good to include this in the PR, otherwise users will set the timeout to 30 seconds and then get confused when the socket times out.

Could you check and change the Sockets too?

@bl0ggy
Copy link
Contributor Author

bl0ggy commented Feb 19, 2020

What is the default Socket Timeout period ?
Because from cam.js the default is 120s.

If in your soft you set a timeout value less than 30s in Cam constructors, that is probably why you have an error. I actually encountered that error too, that's one reason why I have made this PR.

Now in the constructor we can set both a timeout for standard requests and a eventsTimeout for PullMessages.
Every HTTP request uses a new Socket. When we call http.request() here the socket is created and has its timeout set to reqOptions.timeout.

What I did is:

  • Change the _request function to use options.timeout instead of the default one
  • Let us set in the constructor the timeout eventsTimeout used for the PullMessages request
  • Use that eventsTimeout in the _request function from _pullMessages

This is what's happening:

  • We create a Cam object with both timeout and eventsTimeout
  • In the constructor this.events.timeout is set to eventsTimeout
  • We listen to 'event'
  • This will create a pullpoint subscription
  • Then call _eventPull which calls _pullMessages with this.events.timeout as timeout here
  • In _pullMessages this._request is called using options.timeout which corresponds to this.events.timeout+5s here

I added 5 seconds for security, 1 seconds should be enough. I even have cameras that send the PullMessagesResponse ~1s before the timeout.

That made me notice that I forgot about the timeouts of CreatePullPointSubscription and Renew.
Should we set them to double this.events.timeout ?

Could you please try it and tell me if it works for you ? Maybe I am all wrong about the socket timeout.

@RogerHardiman
Copy link
Collaborator

Just a general comment for this PR and your others.
They all look good so thanks for the PRs.

I've got a load of work on at the moment so it may be a while before I get through them all.
@agsh (Andrew) may take a look as well.

So don't worry if you don't see any merges until next week. I've just for a load of things to do for work and over the weekend.

Thanks
Roger

@bl0ggy
Copy link
Contributor Author

bl0ggy commented Feb 20, 2020

You are welcome :)
I worked with this library for a few months already, I made many changes, and some others are coming, I just have to make the JSDoc and check that linting and tests don't fail.

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