Fix handler for multiple calls of -[FIRInstanceID instanceIDWithHandler:] (#2445)

[maksymmalyhin]

This is a fix for #2445:

• -[FIRInstanceID instanceIDWithHandler:] accumulate handlers while there is ongoing request to perform all of them on completion

[charlotteliang]

Do we still need to check this flag or just check if defaultTokenFetchHandler.count > 1?

[maksymmalyhin]

Yeah, my first try was to replace isFetchingDefaultToken by checking defaultTokenFetchHandler.count, but then it is hard to handle retry logic due to the way it is implemented. Also, in this case the handler cannot be optional (which is the case e.g. in #2562). Keeping both, the flag and defaultTokenFetchHandler allows to make less changes to the existing logic.

[charlotteliang]

I feel like you can also get rid of isDefaultTokenFetchScheduled. Because you have the first original call back in and retry just calling with nil.

But if you don't feel comfortable, you can leave it like this for now.

[maksymmalyhin]

As far as I can see, currently isDefaultTokenFetchScheduled prevents additional requests to be sent while [self retryIntervalToFetchDefaultToken] timeout, so, I think, we still need it.

[charlotteliang]

I think since we are adding a new logic to the existing code that could replace the old boolean value. We should do that. We are trying to simplify the iid logic especially your new queue should be able to handle it.

So one way we can do is to have a defaultTokenWithRetry:(BOOL) handler:
So that if it's retry, we always go through, without adding a new handler to the queue. Otherwise, adding it to the queue and check if it's >1.

[maksymmalyhin]

I pushed an update. Would you take a look if it is what you meant?

[maksymmalyhin]

@chliangGoogle Thank you for the attentive review! The logic looks to me a bit difficult to follow with the current implementation. E.g. defaultTokenWithHandler: method currently is responsible for 3 separate things:

1. Fetching the token itself
2. Retry
3. Merge token requests

As I mentioned, I would like to discuss possible ways to improve the code.

[charlotteliang]

Looking good. With a few nit picking.

[charlotteliang]

You might have to merge, this function would not be needed.

[charlotteliang]

we can get rid of defaultTokenWithHandler: and use the new one instead every where.

[maksymmalyhin]

I would prefer not to use defaultTokenWithRetry:handler: everywhere because the retry argument is kind of an implementation detail of defaultTokenWithHandler: method.

@chliangGoogle Let me know if you still would prefer to get rid of it, I'll create a separate PR for it then.