pubsub: allow setting a pull HTTP timeout #981
Conversation
googlebot
added
the
cla: yes
stephenplusplus
force-pushed
the
spp--pubsub-timeout
branch
2 times, most recently
from
b9deb2f to
3ed23db
Compare
| this.maxInProgress = | ||
| is.number(options.maxInProgress) ? options.maxInProgress : Infinity; | ||
| this.messageListeners = 0; | ||
| this.paused = false; | ||
| this.timeout = options.timeout; |
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
stephenplusplus
force-pushed
the
spp--pubsub-timeout
branch
from
3ed23db to
8299c84
Compare
|
@callmehiphop @mattraby -- I updated the PR. See the opening post for details. Let me know what you think! |
|
@tmatsuo read over the issue #980 and the originating StackOverflow issue that inspired this PR -- is this a good idea or not so much? Thanks! |
|
With the addition of #979, is this still necessary? |
|
If we consider this feature useful (still TBD -- need help with that decision), then I think we're just making it easy with this. It would be a bit awkward for the user to do it themselves. |
|
It sounds like we can't resolve the original issue via the client, however imo this is still a nice feature and it's one that could potentially benefit all of the other services. |
|
Oh, gotcha. Supporting easily configurable timeouts is a good idea and request interceptors would be the right way to do that on a global basis. But I still consider that an advanced need and a bit of a learning curve to understand how to do it with our library. In this case, only the HTTP ':pull' request should be affected, so I could see both making sense. Related: #446 (comment) |
stephenplusplus
force-pushed
the
spp--pubsub-timeout
branch
2 times, most recently
from
46de268 to
c540b31
Compare
stephenplusplus
force-pushed
the
spp--pubsub-timeout
branch
from
c540b31 to
8c4d5fb
Compare
| * @param {string} options.encoding - When pulling for messages, this type is | ||
| * used when converting a message's data to a string. (default: 'utf-8') | ||
| * @param {number} options.interval - Interval in milliseconds to check for new | ||
| * messages. | ||
| * messages. (default: 10) | ||
| * @param {number} options.maxInProgress - Maximum messages to consume |
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
|
@stephenplusplus The PR makes sense to me. |
stephenplusplus
force-pushed
the
spp--pubsub-timeout
branch
from
8c4d5fb to
a78b77d
Compare
|
LGTM |
pubsub: allow setting a pull HTTP timeout
Fixes #980
We can't control the server's timeout setting through the API, but I tried to effectively recreate it. The user can cancel an outstanding pull request by breaking the HTTP connection. So this PR lets them set a timeout on the HTTP request. When that amount of time is reached, the request will be broken.
If we don't get a response from the server in that amount of time, we handle the "ETIMEDOUT" error by assuming because we haven't heard anything, there aren't any messages to return. Thus, we return an empty array of messages to the user.