From fb984a22d191949fd7f998dbf1f746a9ee0a6724 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Konstantin=20K=C3=A4fer?= Date: Fri, 8 May 2015 11:30:26 +0200 Subject: [PATCH 1/2] don't trigger a HTTP request twice --- src/mbgl/storage/default_file_source.cpp | 4 ++- test/storage/http_issue_1369.cpp | 44 ++++++++++++++++++++++++ test/test.gypi | 1 + 3 files changed, 48 insertions(+), 1 deletion(-) create mode 100644 test/storage/http_issue_1369.cpp diff --git a/src/mbgl/storage/default_file_source.cpp b/src/mbgl/storage/default_file_source.cpp index ddfcc8984..690a96881 100644 --- a/src/mbgl/storage/default_file_source.cpp +++ b/src/mbgl/storage/default_file_source.cpp @@ -101,9 +101,11 @@ void DefaultFileSource::Impl::startCacheRequest(const Resource& resource) { cache->get(resource, [this, resource](std::unique_ptr response) { DefaultFileRequest* request = find(resource); - if (!request) { + if (!request || request->request) { // There is no request for this URL anymore. Likely, the request was canceled // before we got around to process the cache result. + // The second possibility is that a request has already been started by another cache + // request. In this case, we don't have to do anything either. return; } diff --git a/test/storage/http_issue_1369.cpp b/test/storage/http_issue_1369.cpp new file mode 100644 index 000000000..44c190c82 --- /dev/null +++ b/test/storage/http_issue_1369.cpp @@ -0,0 +1,44 @@ +#include "storage.hpp" + +#include + +#include +#include + +// Test for https://github.com/mapbox/mapbox-gl-native/issue/1369 +// +// A request for http://example.com is made. This triggers a cache get. While the cache get is +// pending, the request is canceled. This removes it from pending. Then, still while the cache get +// is pending, a second request is made for the same resource. This adds an entry back to pending +// and queues another cache request, even though the first one is still pending. Now both cache +// requests resolve to misses, resulting in two HTTP requests for the same resource. The first one +// will notify as expected, the second one will have bound a DefaultFileRequest* in the lambda that +// gets invalidated by the first notify's pending.erase, and when it gets notified, the crash +// occurs. + +TEST_F(Storage, HTTPIssue1369) { + SCOPED_TEST(HTTPIssue1369) + + using namespace mbgl; + + SQLiteCache cache; + DefaultFileSource fs(&cache); + + const Resource resource { Resource::Unknown, "http://127.0.0.1:3000/test" }; + + auto req = fs.request(resource, uv_default_loop(), [&](const Response&) { + ADD_FAILURE() << "Callback should not be called"; + }); + fs.cancel(req); + fs.request(resource, uv_default_loop(), [&](const Response &res) { + EXPECT_EQ(Response::Successful, res.status); + EXPECT_EQ("Hello World!", res.data); + EXPECT_EQ(0, res.expires); + EXPECT_EQ(0, res.modified); + EXPECT_EQ("", res.etag); + EXPECT_EQ("", res.message); + HTTPIssue1369.finish(); + }); + + uv_run(uv_default_loop(), UV_RUN_DEFAULT); +} diff --git a/test/test.gypi b/test/test.gypi index 9c6abb7d3..b6afb4fd9 100644 --- a/test/test.gypi +++ b/test/test.gypi @@ -66,6 +66,7 @@ 'storage/http_coalescing.cpp', 'storage/http_error.cpp', 'storage/http_header_parsing.cpp', + 'storage/http_issue_1369.cpp', 'storage/http_load.cpp', 'storage/http_noloop.cpp', 'storage/http_other_loop.cpp', From bd8d10bab68cff8cdd7cebf2629a7fe6b20c08e5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Konstantin=20K=C3=A4fer?= Date: Fri, 8 May 2015 11:34:46 +0200 Subject: [PATCH 2/2] make DefaultFileRequest movable only --- src/mbgl/storage/default_file_source.cpp | 2 +- src/mbgl/storage/default_file_source_impl.hpp | 8 +++++++- 2 files changed, 8 insertions(+), 2 deletions(-) diff --git a/src/mbgl/storage/default_file_source.cpp b/src/mbgl/storage/default_file_source.cpp index 690a96881..cc7475d2f 100644 --- a/src/mbgl/storage/default_file_source.cpp +++ b/src/mbgl/storage/default_file_source.cpp @@ -85,7 +85,7 @@ void DefaultFileSource::Impl::add(Request* req) { return; } - request = &pending.emplace(resource, DefaultFileRequest(resource)).first->second; + request = &pending.emplace(resource, resource).first->second; request->observers.insert(req); if (cache) { diff --git a/src/mbgl/storage/default_file_source_impl.hpp b/src/mbgl/storage/default_file_source_impl.hpp index ed2d248d0..3845014e9 100644 --- a/src/mbgl/storage/default_file_source_impl.hpp +++ b/src/mbgl/storage/default_file_source_impl.hpp @@ -19,8 +19,14 @@ struct DefaultFileRequest { std::set observers; RequestBase* request = nullptr; - DefaultFileRequest(const Resource& resource_) + inline DefaultFileRequest(const Resource& resource_) : resource(resource_) {} + + // Make it movable-only + DefaultFileRequest(const DefaultFileRequest&) = delete; + inline DefaultFileRequest(DefaultFileRequest&&) = default; + DefaultFileRequest& operator=(const DefaultFileRequest&) = delete; + inline DefaultFileRequest& operator=(DefaultFileRequest&&) = default; }; class DefaultFileSource::Impl {