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

Issue 1582 - DefaultFileSystem should implement atomic write operations #5

Closed
wants to merge 320 commits into from

Conversation

@Mailkov
Copy link
Contributor

commented Jul 2, 2015

Issue 1582 - DefaultFileSystem should implement atomic write operations

palant and others added 30 commits Apr 3, 2013
--HG--
extra : rebase_source : c3573374f7e9e92e647dbd063d1639f5da816cba
--HG--
extra : rebase_source : 7949b89880795537020ad8e54e431f70693be463
--HG--
extra : rebase_source : 2eb37871f96f6e6d6741a25f53411edfcf9d70f2
--HG--
extra : rebase_source : 5bf296f969f0efc189714023bc0a8114dc7424d6
We did this to avoid undefined behaviour in both pthreads and Win32
threads, but this was actually prone to race conditions. Destroying a
mutex or condition variable still in use is a bug that needs fixing
anyway.

--HG--
extra : rebase_source : 2638c2df9d4d9ab9ae5bfc107b42f0869489bf3d
--HG--
extra : rebase_source : dc103979b8d336ba3e43cdd42ee76dc6766df142
--HG--
extra : rebase_source : 0d5342824283607b3bf180bf662625967d48ea03
--HG--
extra : amend_source : d5cfc2486160fc87e863a11e545db75459f8d7b8
--HG--
extra : rebase_source : d1da1794b415d663f2dc3971e9c4c6036cba9fda
--HG--
extra : rebase_source : fa70b5f3f16d65c145a1d8a727a3605af1c6b5bd
--HG--
extra : rebase_source : a5f9d40075847727bfae08a6558ff8677d92c212
…son)

--HG--
extra : rebase_source : 3d7aca0522c79a86939d80becd81dbd0de4708ab
…s in http://codereview.adblockplus.org/10100009/

--HG--
extra : rebase_source : 6c0bf55a0e631b9c7215601a4d1f1a5b944fecad
--HG--
extra : amend_source : c94813671568f9f5ad0688a837b455fd438c2024
--HG--
extra : rebase_source : ca0dec8290e04906da2e6b71c689dd0fa065d2c5
…electors

--HG--
extra : rebase_source : f445894e5fe8fa364e48669a424a04be1b10ac2e
--HG--
extra : rebase_source : a635de824aabe3723d906c143ca9cc7d54276dda
@LaughingGor

This comment has been minimized.

Copy link

commented on bf58ed5 Mar 11, 2015

i have got this project,when i build it, i found a lot of problem.(win7,vs2013).
now i got this erro:

class Persistent declared in v8.h
template<class T, class M = NonCopyablePersistentTraits > class Persistent;
fails with
v8\include\v8.h(105) : error C2977: 'v8::Persistent' : too many template arguments

in V8ValueHolder.h, i found this:
namespace v8
{
class Isolate;
template class Handle;
template class Persistent;
}

is this with the wrong usage?

This comment has been minimized.

Copy link
Member Author

replied Mar 12, 2015

No, it's just that we don't support VS2013 yet, please see: https://issues.adblockplus.org/ticket/1197

This comment has been minimized.

Copy link

replied Mar 16, 2015

as far as i know, v8 can only be builded in vs2013 now. i have tried it in vs2012, v8 can not be builded.(because of v8 used c++11 Variadic Templates, vs2012 didn't support it,vs2013 support this)
when i use vs2013, i can build v8, but can not build libadblockplus, it return the error above.
so i think my v8's version is not right, can you tell me whitch version of v8 you are using.
in your environment, did you use vs2012 and the latest version of v8 to build your own project?

@abby-sergz
abby-sergz reviewed Jul 3, 2015
View changes
src/DefaultFileSystem.cpp Outdated
@@ -78,8 +78,23 @@ DefaultFileSystem::Read(const std::string& path) const
void DefaultFileSystem::Write(const std::string& path,
std::tr1::shared_ptr<std::istream> data)
{
std::ofstream file(NormalizePath(path).c_str(), std::ios_base::out | std::ios_base::binary);
file << Utils::Slurp(*data);
std::wstring pathnormalized = NormalizePath(path);

This comment has been minimized.

Copy link
@abby-sergz

abby-sergz Jul 3, 2015

Member

On not WIN32 NormalizePath returns std::string not std::wstring.

@abby-sergz
abby-sergz reviewed Jul 3, 2015
View changes
src/DefaultFileSystem.cpp Outdated
file.close();
if (file.good())
{
rename(tempfile.c_str(), pathnormalized.c_str());

This comment has been minimized.

Copy link
@abby-sergz

abby-sergz Jul 3, 2015

Member

What's about using DefaultFileSystem::Move here?

This comment has been minimized.

Copy link
@abby-sergz

abby-sergz Jul 3, 2015

Member

I think we need something to reduce std::string <> std::wstring indirection here and we should not create a code duplication.

@abby-sergz
abby-sergz reviewed Jul 3, 2015
View changes
src/DefaultFileSystem.cpp Outdated
std::wstring filename = pathnormalized.substr(0, poslastpoint - 1);
std::wstring tempfile = filename + L".tmp";
std::ofstream file(tempfile.c_str(), std::ios_base::out | std::ios_base::binary);
if (file.good())

This comment has been minimized.

Copy link
@abby-sergz

abby-sergz Jul 3, 2015

Member

I also think about getting rid of these nested scopes.

if (!file.good())
  return;
file << Utils::Slurp(*data);
if (!file.good())
  return;
file.close();
if (!file.good())
  return;

We might should replace return here by throwing of an exception. @fhd

Issue 1582 - fix return wstring or string
@Mailkov

This comment has been minimized.

Copy link
Contributor Author

commented Jul 3, 2015

I think is now fixed ...

@Mailkov
Mailkov reviewed Jul 3, 2015
View changes
src/DefaultFileSystem.cpp Outdated
file.close();
if (!file.good())
return;
rename(tmpfile(NormalizePath(path)).c_str(), NormalizePath(path).c_str());

This comment has been minimized.

Copy link
@Mailkov

Mailkov Jul 3, 2015

Author Contributor

this line is not a duplicated of function DefaultFileSystem::Move
because DefaultFileSystem::Move accepts only std::string ...

@abby-sergz
abby-sergz reviewed Jul 6, 2015
View changes
src/DefaultFileSystem.cpp Outdated
{
std::ofstream file(NormalizePath(path).c_str(), std::ios_base::out | std::ios_base::binary);
{
std::ofstream file(tmpfile(NormalizePath(path)).c_str(), std::ios_base::out | std::ios_base::binary);

This comment has been minimized.

Copy link
@abby-sergz

abby-sergz Jul 6, 2015

Member
  • What about NormalizePath(tmpfile(path))? Then we don't need _wtmpfile.
  • It's better to store the name of temporary file (result of tmpfile(path)) in a local variable instead of generating it again for moving below. So, it seems we can use DefaultFileSystem::Move in this case.
@abby-sergz
abby-sergz reviewed Jul 6, 2015
View changes
src/DefaultFileSystem.cpp Outdated
@@ -75,11 +76,35 @@ DefaultFileSystem::Read(const std::string& path) const
return result;
}

std::string tmpfile(std::string pathnormalized)

This comment has been minimized.

Copy link
@abby-sergz

abby-sergz Jul 6, 2015

Member
  • it should be in an anonymous namespace
  • the argument should be const std::string&

This comment has been minimized.

Copy link
@abby-sergz

abby-sergz Jul 6, 2015

Member

Actually I guess we can get rid of this method and put it in DefaultFileSystem::Write if we don't need a second version for wide string. Nevertheless, by the way, we start method names with a capital letter.

@abby-sergz
abby-sergz reviewed Jul 6, 2015
View changes
src/DefaultFileSystem.cpp Outdated
{
int poslastpoint = pathnormalized.find_last_of(".");
std::string filename = pathnormalized.substr(0, poslastpoint - 1);
std::string tempfile = filename + ".tmp";

This comment has been minimized.

Copy link
@abby-sergz

abby-sergz Jul 6, 2015

Member

Let's merely add the suffix .tmp, not replace.

Issue 1582 - use Tmpfile function
@Mailkov

This comment has been minimized.

Copy link
Contributor Author

commented Jul 7, 2015

I hope it is now ok ...

std::string Tmpfile(const std::string& path)
{
std::string pat = path;
return pat += ".tmp";

This comment has been minimized.

Copy link
@abby-sergz

abby-sergz Jul 7, 2015

Member

It can be just

{
  return path + ".tmp";
}

However, I would proceed without this function so far.

@abby-sergz

This comment has been minimized.

Copy link
Member

commented Jul 7, 2015

Except Tmpfile function and the question about throwing of exceptions (@fhd) instead merely return; it LGTM.

@fhd fhd force-pushed the adblockplus:master branch from d9a9a15 to 0d650b1 Aug 11, 2015
@fhd fhd closed this Aug 13, 2015
@fhd fhd force-pushed the adblockplus:master branch from 5934cb6 to 844dd77 Aug 13, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.