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

[ios][mac] osm auth test fix #7822

Merged
merged 1 commit into from
Apr 6, 2024
Merged

[ios][mac] osm auth test fix #7822

merged 1 commit into from
Apr 6, 2024

Conversation

biodranik
Copy link
Member

@biodranik biodranik commented Apr 5, 2024

The issue is that tests are blocked by the main thread.

@biodranik biodranik requested a review from strump April 5, 2024 13:17
@vng
Copy link
Member

vng commented Apr 5, 2024

Did you check this session callbacks for possible synchronization or races?
IDK where this HTTP session is used now, but I remember that we perform file writing in downloader callback without sync.

@biodranik
Copy link
Member Author

@vng the initial implementation was wrong. This whole downloader implementation should be refactored.
For iOS, we have a chunked downloader (not used now), and a synchronous one (used in sync calls to auth for example).
Chunked downloaded should use the same thread to write files and to modify internals. It may be a different thread, not the main one. But iOS API doesn't allow to explicitly specify threads.

Android HTTP still does not support HTTP/2 (and of course HTTP/3 too), so it may be a good time to refactor everything and get better download speed and less battery consumption.

@biodranik biodranik changed the title [ios][mac] Do not use main thread for HTTP callbacks [ios][mac] osm auth test fix Apr 6, 2024
Copy link
Sponsor Contributor

@strump strump left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can replace OsmOAuth::DevServerAuth() with OsmOAuth::ServerAuth() to have release or debug OSM host depending on DEBUG flag.

TEST_EQUAL(result, false, ("invalid password"));
TEST(!auth.IsAuthorized(), ("Should not be authorized."));
});
OsmOAuth auth = OsmOAuth::DevServerAuth();
Copy link
Sponsor Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
OsmOAuth auth = OsmOAuth::DevServerAuth();
OsmOAuth auth = OsmOAuth::ServerAuth();

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@strump

Running osm_auth_tests.cpp::OSM_Auth_Login
FAILED
osm_auth_tests/osm_auth_tests.cpp:30 Unexpected exception at TEST(result = auth.AuthorizePassword(kValidOsmUser, kValidOsmPassword)) HTTP 401

Can you please help to debug what is wrong? We need some "test" credentials for the prod.

TEST_EQUAL(perm.first, OsmOAuth::HTTP::OK, ("permission request ok"));
TEST_NOT_EQUAL(perm.second.find("write_api"), std::string::npos, ("can write to api"));
});
OsmOAuth auth = OsmOAuth::DevServerAuth();
Copy link
Sponsor Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
OsmOAuth auth = OsmOAuth::DevServerAuth();
OsmOAuth auth = OsmOAuth::ServerAuth();

@@ -58,6 +59,8 @@ - (instancetype)init
delegateQueue:nil];
_taskInfoByTaskID = [NSMutableDictionary dictionary];
_taskInfoQueue = dispatch_queue_create("http_session_manager.queue", DISPATCH_QUEUE_CONCURRENT);
BOOL const isSyncHttpTest = [NSBundle.mainBundle.executableURL.lastPathComponent hasSuffix:@"osm_auth_tests"];
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks strange.

Is it possible to detect that we are already in main queue while performing dispatch_async(dispatch_get_main_queue()
I suppose that ObjC core should do it itself, something like coroutine concept.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The main thread can be detected by NSThread.isMainThread, but it doesn't help here. The delegate is called from another thread. Old code posted a block to the main thread that was blocked in the test by dispatch_group_wait(group, DISPATCH_TIME_FOREVER); so the posted block was never executed.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok then, if we already have taskInfoQueue for processing task infos, why we can't use it for running completionHandler always? Especially, because we use taskInfo.delegate and task itself to run handler.

Otherwise, assume calling dispatch taskInfo.delegate to dispatch_get_main_queue and calling removeTaskInfoForTask at the same time?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because Apple's dispatch queues are tricky. There is no guarantee that any queue except the main one will run on the same thread. The same queue can reuse different threads depending on their availability. And we have a lot of thread checkers in chunker downloader for example to make sure that its callbacks are always single-threaded.

Work submitted to dispatch queues executes on a pool of threads managed by the system. Except for the dispatch queue representing your app's main thread, the system makes no guarantees about which thread it uses to execute a task.

source

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This all mess should be rewritten properly using thread pools and lambdas )

Copy link
Member

@vng vng left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, I'd put todo explaining isSyncHttpTest

…sm_auth_tests

Signed-off-by: Alexander Borsuk <me@alex.bio>
@biodranik biodranik merged commit edb7fc2 into master Apr 6, 2024
14 checks passed
@biodranik biodranik deleted the ab-fix-auth-tests branch April 6, 2024 22:46
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.

None yet

3 participants