Re: issue#435, ensure that duplicate calls to destroy arcore are ignored

[kmayoral]

Since it takes a few seconds to destroy the arcore object, multiple calls to the method can still happen concurrently while the first is still being destroyed but before the object reference has been nullified. This should protect against that race condition.

[sameerjj]

great work! hopefully this fixes it once and for all.

[Antiglobalist]

@kmayoral Hi!
As you can see in my PR I removed destroyARCore() from override fun destroy()
#438
I wrote a comment that I'm not sure if I need to call this in two places

Now I see that in version 2.1.0 the second call was returned. Perhaps the one who returned it understands the logic better and this call is necessary. But, apparently because of this, the crash returned again

I see your fix. You have added a flag
But maybe one of the owners will explain us, why do we need the second call of destroy?

I see only one case, when ARScene view will not have lifecycle

I think next (it is not a best solution) solution can prevent double destroy also

I don't know if it is possible use ArSceneView without Lifecycle
My opinion is that the design code of this class has an error
We can pass sharedLifecycle: Lifecycle? = null to constructor. And it calls -

Then we can also setup lifecycle, and it calls also -

If i see right this case will call twice Core logic -

It can produce new error in the future
What do you think?

[Antiglobalist]

So, the main questions:
1 - Do we really need to be able to pass two different Lifecycles?
2 - Is it possible to leave one and make it mandatory?

I don't understand why they are needed in construcor. For compose view ?

sharedActivity: ComponentActivity? = null,
    sharedLifecycle: Lifecycle? = null,

Is it possible to change the subscription method to this? Then there will be one lifecycle

Cuz it looks like only 1 Lifecycle is necessary for ArCore calls

[kmayoral]

Hi @Antiglobalist , thanks for the comments! So it seems that your changes were probably wiped by a conflict resolving merge somewhere along the lines.

I definitely wouldn't be opposed to both changes being included in the code, but I do feel that this PR will make it so any other accidental double calls of destroy() don't cause issues by making this action more idempotent to begin with.

I would like if we could land this PR and then have another PR that reopens the changes you had previously made, what do you think?

[Antiglobalist]

@kmayoral Hi!
Sure!
I just noticed a common problem :)

[Antiglobalist]

@grassydragon Hello. Please take a look at my research above.
Perhaps it makes sense for the future

[ThomasGorisse]

Thanks!

[ThomasGorisse]

@Antiglobalist The double Lifecycle was there for Flutter but it might not be applicable anymore.
So could you please make it mandatory and unique in another PR?

[Antiglobalist]

@ThomasGorisse Sure!