Open bl0ggy opened 4 years ago
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?
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:
_request
function to use options.timeout
instead of the default one_request
function from _pullMessages
This is what's happening:
timeout
and eventsTimeout
this.events.timeout
is set to eventsTimeout
_eventPull
which calls _pullMessages
with this.events.timeout
as timeout here_pullMessages
this._request
is called using options.timeout
which corresponds to this.events.timeout
+5s hereI 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.
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
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.
Add the possibility to choose the timeout of a PullMessagesRequest. Also add the possibility to choose the timeout of a http request.