Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Set default timeout to 10 minutes #230

Merged
merged 3 commits into from
Dec 8, 2022

Conversation

FrikkieSnyman
Copy link
Contributor

@FrikkieSnyman FrikkieSnyman commented Dec 8, 2022

Overview

On Android, if we call getCurrentPosition using the "android" provider, we default to Long.MAX_VALUE if no timeout is passed.

In turn, this timeout gets passed to mHandler.postDelayed, which is meant to reject the call once the timeout has been reached.

However, postDelayed makes use of sendMessageDelayed under the hood, which then finally calls sendMessageAtTime - using the delay and adding SystemClock.uptimeMillis() to it.

Essentially, using Long.MAX_VALUE causes an overflow here, and leads to the reject timeout triggering immediately.

It could be argued that having an infinite timeout as default does not make sense anyway, and setting it to 10 minutes is much more sensible.

Test Plan

Call getCurrentPosition with no timeout specified - it should return a location instead of rejecting with Location request timed out.

ios/RNCGeolocation.mm Outdated Show resolved Hide resolved
@michalchudziak michalchudziak merged commit 129d044 into michalchudziak:master Dec 8, 2022
@FrikkieSnyman FrikkieSnyman deleted the default-timeout branch December 8, 2022 17:19
@FrikkieSnyman
Copy link
Contributor Author

https://issuetracker.google.com/issues/181711691

Android is not planning on changing this behaviour to sendMessageDelayed:

This is working as intended. The caller shouldn't use the Long.MAX_VALUE as the delay, as it'll guarantee there will be an overflow. And what's the point to schedule a handler with infinity delay?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants