feat: make publish futures compatible with concurrent.futures.as_completed()

[plamut]

Closes #368.

After studying the as_completed() code from the standard library, it actually seemed easier to modify our futures implementation, as opposed to writing our own as_completed() logic, which would still require changes to the Future, too (think: locking and notifying).

The drawback of course is relying on implementation details from concurrent.futures, but good tests should help us here. Let's discuss this drawback here.

Current approach pros:

• Fewer code changes, no need to write our own as_completed() logic.
• The library's API surface does not change, no incompatibility with other official Pub/Sub clients, no dilemma and coordination on whether to move the implementation to api-core.

Current approach cons:

• Compatibility depends on concurrent.futures implementation details, although tests should help here. In addition, the standard lib's implementation seems very stable, and changes infrequent (blame) - the key implementation details are more than 10 years old.

PR checklist:

• Make sure to open an issue as a bug/issue before writing your code! That way we can discuss the change, evaluate designs, and agree on the general idea
• Ensure the tests and linter pass
• Code coverage does not decrease (if any source code was changed)
• Appropriate docs were updated (if necessary)

[plamut]

@pradn PTAL, if you have time.

It's true that the solution uses concurrent.futures implementation details, but the required changes are relatively small and we'll have tests to protect us.

Alternatively, we could provide our own as_completed helper, but that would likely require a bit more code, including the changes to our Future itself, plus we would be adding new stuff to the library surface that other clients probably don't have.

After reading the standard library implementation, the first approach actually seemed more favourable (IMHO).

[pradn]

I can take a look later this week or early next week. :)

[pradn]

I took a first pass - looks reasonable. We need to check with Kamal if depending on the internals of a library class like this is fine. It seems to be helpful enough to the user to warrant this, but I wonder if so few are going to use this it might not be worth it.

[plamut]

Using as_completed() is pretty standard when dealing with multiple futures and waiting on their completion, it's not that niche a use case IMHO.

IIRC the feature request has been confirmed (and Node is adding something similar, too), but I agree we should discuss the approach in this PR - I'll add it to the agenda.

[pradn]

Looks good to me. It seems like we have consensus with the rest of the team, so perhaps simply ask for confirmation before merging?

[plamut]

Looks good to me. It seems like we have consensus with the rest of the team, so perhaps simply ask for confirmation before merging?

That's the plan.

And we might also borrow @jimfulton to have a say on using those implementation details, as he's been around Python ... probably since the fall of the Berlin Wall. 😆

(which might actually not even be far from the truth)

Decided to also try swapping the futures with std lib's implementation first

[plamut]

After an offline discussion, we think relying on implementation details is not desirable, and the approach highest on the wish list is to swap the current implementation in the library with futures from concurrent.futures package.

The public interface is already compatible, but we need to see if that would break Pub/Sub Lite, and also determine the reason why the original authors decided to use their own implementation.

@dpcollins-google Does the Pub/Sub Lite client use any internals of the Futures, or just their public interface? Would replacing the futures with the implementation from concurrent.futures break any assumptions?

[plamut]

Closing in favor of #397. We prefer less code and don't want to rely on the implementation details of concurrent.futures.Future.