pubsub: cap stream reconnect time

[pongad]

Previously we exponentially grow stream reconnect backoff time
without bound, so unlucky users will wait forever waiting for
a connection.
This commit caps the reconnect at 10s (the same as polling).

Previously we only reset the polling time after the stream closes
without error. This usually doesn't happen because the streams are
long running and don't close until some error is encountered.
This commit resets the polling time if we receive any response
from the stream.

Updates #2406.

[pongad]

@davidtorres Looking at this more, I'm not sure if this class is properly synchronized. Do we need a mutex around requestObserver?

I think MessageDispatcher could try to ack at the same time initialize tries to reestablish a stream. I think we can get a read-write race on the requestObserver variable?

[Andy2003]

Additionally the Builder for the Subscriber should be fixed, so that the user can chosse between polling or streaming. See: https://github.com/GoogleCloudPlatform/google-cloud-java/blob/ce59d34fa59c66add4c8370f1ca760f127a91b56/google-cloud-pubsub/src/main/java/com/google/cloud/pubsub/v1/Subscriber.java#L607 (should be public)

[pongad]

@Andy2003 The private access was intentional. We'd like to migrate to streaming exclusively. Do you have a use case that streaming does not solve? If there is, I think we should consider that a bug.

[Andy2003]

A solution to work-around the current bug would be to switch back to pulling. But this is not possible, so the only choice was to downgrade to 0.21.1-beta.

[pongad]

If you are running into this in production, I think the right answer is to roll back instead of forward. It does take some time for us to cut a new release so even with this PR merged, we might not be able to cut one immediately.

That said, it might make sense, in the next release, to make this method public and @Deprecated until more bugs in streaming pull is flushed out. @davidtorres What do you think of this?

[mdietz94]

@davidtorres is OOO this week FYI. I would be against marking deprecated, the message should be that StreamingPull is the new preferred API - the first users to upgrade to a new version will always be taking on some amount of risk, and I think rolling back makes more sense than keeping the old version of the code alive just in case there are bugs - we discuss more offline if you feel strongly.

[pongad]

@mdietz94 I agree with you but wanted to bring others into the conversation since I don't want to be a dictator :)

Would you like to review this change? Or should we wait for David?

[pongad]

@mdietz94 PTAL. The failing test seems to be coming from logging and unrelated to this change. Do you have any opinion about the potential race I called out earlier?

[mdietz94]

I agree it looks like there could be a race, unless there is some other sort of guarantee that we won't try to ack while restarting a stream. I think a Mutex would definitely fix it, but maybe it's worth looking through the initial review, I didn't take part in it and maybe they discussed why this wouldn't e a problem then.

If it isn't a race it definitely needs a comment explaining why.

[pongad]

@mdietz94 I looked into this more. I'm almost certain there's a race. I'll merge this PR now and send out a fix separately.