avoid race condition during chunk write

[sbalmer]

When the chunk file is first removed before the new version
is moved into place, racing reads may encounter a missing chunk.

Using rename() or replace() without remove() avoids the issue
on Posix-Systems as the methods are atomic. The fallback of
remove() -> rename() is included for Windows pre Python 3.3.

Fixes#263

TODO:

• Add unit tests and/or doctests in docstrings
• Add docstrings and API docs for any new/modified user-facing classes and functions
• New/modified features documented in docs/tutorial.rst
• Changes documented in docs/release.rst
• Docs build locally (e.g., run tox -e docs)
• AppVeyor and Travis CI passes
• Test coverage is 100% (Coveralls passes)

[jakirkham]

So this is the case where we would want the pyosreplace package. It's a Windows only backport for os.replace on Python 2.

[alimanfoo]

Just to mention that there are some potential consequences from implementing this PR that we should be aware of, described in #328. I don't think that should stop us from moving ahead with this PR, using os.replace() is clearly a better thing to do under any circumstances, but just something to be aware of.

[sbalmer]

@jakirkham I've made a guess at the setup.py changes needed for pyosreplace. I couldn't get environmental markers to work. See previous commit. Could you please have a look at it? And is something else needed?

[jakirkham]

Think you had it basically right. Unfortunately there is some lack of clarity about when a colon or semicolon should be used. IIUC it should be a semicolon.

[jakirkham]

LGTM. Thanks for working on this @sbalmer.

Hopefully the comment above is useful. Am personally fine with how this is. Though if you want to use environment markers instead (or others prefer this), that could be also done, but don't personally think this is critical.

[alimanfoo]

Thanks @sbalmer. Could you add pyosreplace to requirements.txt and requirements_dev.txt, pinned to the latest version in the latter. I guess environment markers are needed in both.

[sbalmer]

@alimanfoopyosreplace is now in the requirement files too. I've made a guess at how the env markers should look. If I invert the predicates then pip install -r requirements.txt tries to install the package. Which means I have some confidence it should work.

[jakirkham]

Thanks @sbalmer. LGTM. 😄 Let's see what @alimanfoo thinks. 😉

[jakirkham]

I wonder if we can just drop the try...finally now.

[alimanfoo]

Many thanks @sbalmer, looks good. Could you add a release note to docs/release.rst.

[alimanfoo]

I wonder if we can just drop the try...finally now.

FWIW I imagine it could still be possible that some failure occurs during the attempt to write to the temporary file (e.g., device gets full or something like that), in which case an exception occurs before the value has been fully written, in which case we'd still want to clean up the temporary file.

[jakirkham]

Good point.

[alimanfoo]

I've resolved conflicts after merging #352 and added a release note. Will merge if CI passes.

[alimanfoo]

@sbalmer apologies I didn't know your full name to include in the release note, you're credited currently just as "sbalmer", happy to leave it that way or add your full name, whichever you prefer.

[jakirkham]

Thanks @sbalmer 😄

[sbalmer]

Thanks @alimanfoo, @jakirkham!

Now we can use mainline again :-)

[sbalmer]

When the chunk file is first removed before the new version
is moved into place, racing reads may encounter a missing chunk.

Using rename() or replace() without remove() avoids the issue
on Posix-Systems as the methods are atomic. The fallback of
remove() -> rename() is included for Windows pre Python 3.3.

Fixes#263

TODO:

• Add unit tests and/or doctests in docstrings
• Add docstrings and API docs for any new/modified user-facing classes and functions
• New/modified features documented in docs/tutorial.rst
• Changes documented in docs/release.rst
• Docs build locally (e.g., run tox -e docs)
• AppVeyor and Travis CI passes
• Test coverage is 100% (Coveralls passes)

[jakirkham]

So this is the case where we would want the pyosreplace package. It's a Windows only backport for os.replace on Python 2.

[alimanfoo]

Just to mention that there are some potential consequences from implementing this PR that we should be aware of, described in #328. I don't think that should stop us from moving ahead with this PR, using os.replace() is clearly a better thing to do under any circumstances, but just something to be aware of.

[sbalmer]

@jakirkham I've made a guess at the setup.py changes needed for pyosreplace. I couldn't get environmental markers to work. See previous commit. Could you please have a look at it? And is something else needed?

[jakirkham]

Think you had it basically right. Unfortunately there is some lack of clarity about when a colon or semicolon should be used. IIUC it should be a semicolon.

[jakirkham]

LGTM. Thanks for working on this @sbalmer.

Hopefully the comment above is useful. Am personally fine with how this is. Though if you want to use environment markers instead (or others prefer this), that could be also done, but don't personally think this is critical.

[alimanfoo]

Thanks @sbalmer. Could you add pyosreplace to requirements.txt and requirements_dev.txt, pinned to the latest version in the latter. I guess environment markers are needed in both.

[sbalmer]

@alimanfoopyosreplace is now in the requirement files too. I've made a guess at how the env markers should look. If I invert the predicates then pip install -r requirements.txt tries to install the package. Which means I have some confidence it should work.

[jakirkham]

Thanks @sbalmer. LGTM. 😄 Let's see what @alimanfoo thinks. 😉

[jakirkham]

I wonder if we can just drop the try...finally now.

[alimanfoo]

Many thanks @sbalmer, looks good. Could you add a release note to docs/release.rst.

[alimanfoo]

I wonder if we can just drop the try...finally now.

FWIW I imagine it could still be possible that some failure occurs during the attempt to write to the temporary file (e.g., device gets full or something like that), in which case an exception occurs before the value has been fully written, in which case we'd still want to clean up the temporary file.

[jakirkham]

Good point.

[alimanfoo]

I've resolved conflicts after merging #352 and added a release note. Will merge if CI passes.

[alimanfoo]

@sbalmer apologies I didn't know your full name to include in the release note, you're credited currently just as "sbalmer", happy to leave it that way or add your full name, whichever you prefer.

[jakirkham]

Thanks @sbalmer 😄

[sbalmer]

Thanks @alimanfoo, @jakirkham!

Now we can use mainline again :-)

[sbalmer]

When the chunk file is first removed before the new version
is moved into place, racing reads may encounter a missing chunk.

Using rename() or replace() without remove() avoids the issue
on Posix-Systems as the methods are atomic. The fallback of
remove() -> rename() is included for Windows pre Python 3.3.

Fixes#263

TODO:

• Add unit tests and/or doctests in docstrings
• Add docstrings and API docs for any new/modified user-facing classes and functions
• New/modified features documented in docs/tutorial.rst
• Changes documented in docs/release.rst
• Docs build locally (e.g., run tox -e docs)
• AppVeyor and Travis CI passes
• Test coverage is 100% (Coveralls passes)

[jakirkham]

So this is the case where we would want the pyosreplace package. It's a Windows only backport for os.replace on Python 2.

[alimanfoo]

Just to mention that there are some potential consequences from implementing this PR that we should be aware of, described in #328. I don't think that should stop us from moving ahead with this PR, using os.replace() is clearly a better thing to do under any circumstances, but just something to be aware of.

[sbalmer]

@jakirkham I've made a guess at the setup.py changes needed for pyosreplace. I couldn't get environmental markers to work. See previous commit. Could you please have a look at it? And is something else needed?

[jakirkham]

Think you had it basically right. Unfortunately there is some lack of clarity about when a colon or semicolon should be used. IIUC it should be a semicolon.

[jakirkham]

LGTM. Thanks for working on this @sbalmer.

Hopefully the comment above is useful. Am personally fine with how this is. Though if you want to use environment markers instead (or others prefer this), that could be also done, but don't personally think this is critical.

[alimanfoo]

Thanks @sbalmer. Could you add pyosreplace to requirements.txt and requirements_dev.txt, pinned to the latest version in the latter. I guess environment markers are needed in both.

[sbalmer]

@alimanfoopyosreplace is now in the requirement files too. I've made a guess at how the env markers should look. If I invert the predicates then pip install -r requirements.txt tries to install the package. Which means I have some confidence it should work.

[jakirkham]

Thanks @sbalmer. LGTM. 😄 Let's see what @alimanfoo thinks. 😉

[jakirkham]

I wonder if we can just drop the try...finally now.

[alimanfoo]

Many thanks @sbalmer, looks good. Could you add a release note to docs/release.rst.

[alimanfoo]

I wonder if we can just drop the try...finally now.

FWIW I imagine it could still be possible that some failure occurs during the attempt to write to the temporary file (e.g., device gets full or something like that), in which case an exception occurs before the value has been fully written, in which case we'd still want to clean up the temporary file.

[jakirkham]

Good point.

[alimanfoo]

I've resolved conflicts after merging #352 and added a release note. Will merge if CI passes.

[alimanfoo]

@sbalmer apologies I didn't know your full name to include in the release note, you're credited currently just as "sbalmer", happy to leave it that way or add your full name, whichever you prefer.

[jakirkham]

Thanks @sbalmer 😄

[sbalmer]

Thanks @alimanfoo, @jakirkham!

Now we can use mainline again :-)

[sbalmer]

When the chunk file is first removed before the new version
is moved into place, racing reads may encounter a missing chunk.

Using rename() or replace() without remove() avoids the issue
on Posix-Systems as the methods are atomic. The fallback of
remove() -> rename() is included for Windows pre Python 3.3.

Fixes#263

TODO:

• Add unit tests and/or doctests in docstrings
• Add docstrings and API docs for any new/modified user-facing classes and functions
• New/modified features documented in docs/tutorial.rst
• Changes documented in docs/release.rst
• Docs build locally (e.g., run tox -e docs)
• AppVeyor and Travis CI passes
• Test coverage is 100% (Coveralls passes)

[jakirkham]

So this is the case where we would want the pyosreplace package. It's a Windows only backport for os.replace on Python 2.

[alimanfoo]

Just to mention that there are some potential consequences from implementing this PR that we should be aware of, described in #328. I don't think that should stop us from moving ahead with this PR, using os.replace() is clearly a better thing to do under any circumstances, but just something to be aware of.

[sbalmer]

@jakirkham I've made a guess at the setup.py changes needed for pyosreplace. I couldn't get environmental markers to work. See previous commit. Could you please have a look at it? And is something else needed?

[jakirkham]

Think you had it basically right. Unfortunately there is some lack of clarity about when a colon or semicolon should be used. IIUC it should be a semicolon.

[jakirkham]

LGTM. Thanks for working on this @sbalmer.

Hopefully the comment above is useful. Am personally fine with how this is. Though if you want to use environment markers instead (or others prefer this), that could be also done, but don't personally think this is critical.

[alimanfoo]

Thanks @sbalmer. Could you add pyosreplace to requirements.txt and requirements_dev.txt, pinned to the latest version in the latter. I guess environment markers are needed in both.

[sbalmer]

@alimanfoopyosreplace is now in the requirement files too. I've made a guess at how the env markers should look. If I invert the predicates then pip install -r requirements.txt tries to install the package. Which means I have some confidence it should work.

[jakirkham]

Thanks @sbalmer. LGTM. 😄 Let's see what @alimanfoo thinks. 😉

[jakirkham]

I wonder if we can just drop the try...finally now.

[alimanfoo]

Many thanks @sbalmer, looks good. Could you add a release note to docs/release.rst.

[alimanfoo]

I wonder if we can just drop the try...finally now.

FWIW I imagine it could still be possible that some failure occurs during the attempt to write to the temporary file (e.g., device gets full or something like that), in which case an exception occurs before the value has been fully written, in which case we'd still want to clean up the temporary file.

[jakirkham]

Good point.

[alimanfoo]

I've resolved conflicts after merging #352 and added a release note. Will merge if CI passes.

[alimanfoo]

@sbalmer apologies I didn't know your full name to include in the release note, you're credited currently just as "sbalmer", happy to leave it that way or add your full name, whichever you prefer.

[jakirkham]

Thanks @sbalmer 😄

[sbalmer]

Thanks @alimanfoo, @jakirkham!

Now we can use mainline again :-)

[sbalmer]

When the chunk file is first removed before the new version
is moved into place, racing reads may encounter a missing chunk.

Using rename() or replace() without remove() avoids the issue
on Posix-Systems as the methods are atomic. The fallback of
remove() -> rename() is included for Windows pre Python 3.3.

Fixes#263

TODO:

• Add unit tests and/or doctests in docstrings
• Add docstrings and API docs for any new/modified user-facing classes and functions
• New/modified features documented in docs/tutorial.rst
• Changes documented in docs/release.rst
• Docs build locally (e.g., run tox -e docs)
• AppVeyor and Travis CI passes
• Test coverage is 100% (Coveralls passes)

[jakirkham]

So this is the case where we would want the pyosreplace package. It's a Windows only backport for os.replace on Python 2.

[alimanfoo]

Just to mention that there are some potential consequences from implementing this PR that we should be aware of, described in #328. I don't think that should stop us from moving ahead with this PR, using os.replace() is clearly a better thing to do under any circumstances, but just something to be aware of.

[sbalmer]

@jakirkham I've made a guess at the setup.py changes needed for pyosreplace. I couldn't get environmental markers to work. See previous commit. Could you please have a look at it? And is something else needed?

[jakirkham]

Think you had it basically right. Unfortunately there is some lack of clarity about when a colon or semicolon should be used. IIUC it should be a semicolon.

[jakirkham]

LGTM. Thanks for working on this @sbalmer.

Hopefully the comment above is useful. Am personally fine with how this is. Though if you want to use environment markers instead (or others prefer this), that could be also done, but don't personally think this is critical.

[alimanfoo]

Thanks @sbalmer. Could you add pyosreplace to requirements.txt and requirements_dev.txt, pinned to the latest version in the latter. I guess environment markers are needed in both.

[sbalmer]

@alimanfoopyosreplace is now in the requirement files too. I've made a guess at how the env markers should look. If I invert the predicates then pip install -r requirements.txt tries to install the package. Which means I have some confidence it should work.

[jakirkham]

Thanks @sbalmer. LGTM. 😄 Let's see what @alimanfoo thinks. 😉

[jakirkham]

I wonder if we can just drop the try...finally now.

[alimanfoo]

Many thanks @sbalmer, looks good. Could you add a release note to docs/release.rst.

[alimanfoo]

I wonder if we can just drop the try...finally now.

FWIW I imagine it could still be possible that some failure occurs during the attempt to write to the temporary file (e.g., device gets full or something like that), in which case an exception occurs before the value has been fully written, in which case we'd still want to clean up the temporary file.

[jakirkham]

Good point.

[alimanfoo]

I've resolved conflicts after merging #352 and added a release note. Will merge if CI passes.

[alimanfoo]

@sbalmer apologies I didn't know your full name to include in the release note, you're credited currently just as "sbalmer", happy to leave it that way or add your full name, whichever you prefer.

[jakirkham]

Thanks @sbalmer 😄

[sbalmer]

Thanks @alimanfoo, @jakirkham!

Now we can use mainline again :-)

[sbalmer]

When the chunk file is first removed before the new version
is moved into place, racing reads may encounter a missing chunk.

Using rename() or replace() without remove() avoids the issue
on Posix-Systems as the methods are atomic. The fallback of
remove() -> rename() is included for Windows pre Python 3.3.

Fixes#263

TODO:

• Add unit tests and/or doctests in docstrings
• Add docstrings and API docs for any new/modified user-facing classes and functions
• New/modified features documented in docs/tutorial.rst
• Changes documented in docs/release.rst
• Docs build locally (e.g., run tox -e docs)
• AppVeyor and Travis CI passes
• Test coverage is 100% (Coveralls passes)

[jakirkham]

So this is the case where we would want the pyosreplace package. It's a Windows only backport for os.replace on Python 2.

[alimanfoo]

Just to mention that there are some potential consequences from implementing this PR that we should be aware of, described in #328. I don't think that should stop us from moving ahead with this PR, using os.replace() is clearly a better thing to do under any circumstances, but just something to be aware of.

[sbalmer]

@jakirkham I've made a guess at the setup.py changes needed for pyosreplace. I couldn't get environmental markers to work. See previous commit. Could you please have a look at it? And is something else needed?

[jakirkham]

Think you had it basically right. Unfortunately there is some lack of clarity about when a colon or semicolon should be used. IIUC it should be a semicolon.

[jakirkham]

LGTM. Thanks for working on this @sbalmer.

Hopefully the comment above is useful. Am personally fine with how this is. Though if you want to use environment markers instead (or others prefer this), that could be also done, but don't personally think this is critical.

[alimanfoo]

Thanks @sbalmer. Could you add pyosreplace to requirements.txt and requirements_dev.txt, pinned to the latest version in the latter. I guess environment markers are needed in both.

[sbalmer]

@alimanfoopyosreplace is now in the requirement files too. I've made a guess at how the env markers should look. If I invert the predicates then pip install -r requirements.txt tries to install the package. Which means I have some confidence it should work.

[jakirkham]

Thanks @sbalmer. LGTM. 😄 Let's see what @alimanfoo thinks. 😉

[jakirkham]

I wonder if we can just drop the try...finally now.

[alimanfoo]

Many thanks @sbalmer, looks good. Could you add a release note to docs/release.rst.

[alimanfoo]

I wonder if we can just drop the try...finally now.

FWIW I imagine it could still be possible that some failure occurs during the attempt to write to the temporary file (e.g., device gets full or something like that), in which case an exception occurs before the value has been fully written, in which case we'd still want to clean up the temporary file.

[jakirkham]

Good point.

[alimanfoo]

I've resolved conflicts after merging #352 and added a release note. Will merge if CI passes.

[alimanfoo]

@sbalmer apologies I didn't know your full name to include in the release note, you're credited currently just as "sbalmer", happy to leave it that way or add your full name, whichever you prefer.

[jakirkham]

Thanks @sbalmer 😄

[sbalmer]

Thanks @alimanfoo, @jakirkham!

Now we can use mainline again :-)

[sbalmer]

When the chunk file is first removed before the new version
is moved into place, racing reads may encounter a missing chunk.

Using rename() or replace() without remove() avoids the issue
on Posix-Systems as the methods are atomic. The fallback of
remove() -> rename() is included for Windows pre Python 3.3.

Fixes#263

TODO:

• Add unit tests and/or doctests in docstrings
• Add docstrings and API docs for any new/modified user-facing classes and functions
• New/modified features documented in docs/tutorial.rst
• Changes documented in docs/release.rst
• Docs build locally (e.g., run tox -e docs)
• AppVeyor and Travis CI passes
• Test coverage is 100% (Coveralls passes)

[jakirkham]

So this is the case where we would want the pyosreplace package. It's a Windows only backport for os.replace on Python 2.

[alimanfoo]

Just to mention that there are some potential consequences from implementing this PR that we should be aware of, described in #328. I don't think that should stop us from moving ahead with this PR, using os.replace() is clearly a better thing to do under any circumstances, but just something to be aware of.

[sbalmer]

@jakirkham I've made a guess at the setup.py changes needed for pyosreplace. I couldn't get environmental markers to work. See previous commit. Could you please have a look at it? And is something else needed?

[jakirkham]

Think you had it basically right. Unfortunately there is some lack of clarity about when a colon or semicolon should be used. IIUC it should be a semicolon.

[jakirkham]

LGTM. Thanks for working on this @sbalmer.

Hopefully the comment above is useful. Am personally fine with how this is. Though if you want to use environment markers instead (or others prefer this), that could be also done, but don't personally think this is critical.

[alimanfoo]

Thanks @sbalmer. Could you add pyosreplace to requirements.txt and requirements_dev.txt, pinned to the latest version in the latter. I guess environment markers are needed in both.

[sbalmer]

@alimanfoopyosreplace is now in the requirement files too. I've made a guess at how the env markers should look. If I invert the predicates then pip install -r requirements.txt tries to install the package. Which means I have some confidence it should work.

[jakirkham]

Thanks @sbalmer. LGTM. 😄 Let's see what @alimanfoo thinks. 😉

[jakirkham]

I wonder if we can just drop the try...finally now.

[alimanfoo]

Many thanks @sbalmer, looks good. Could you add a release note to docs/release.rst.

[alimanfoo]

I wonder if we can just drop the try...finally now.

FWIW I imagine it could still be possible that some failure occurs during the attempt to write to the temporary file (e.g., device gets full or something like that), in which case an exception occurs before the value has been fully written, in which case we'd still want to clean up the temporary file.

[jakirkham]

Good point.

[alimanfoo]

I've resolved conflicts after merging #352 and added a release note. Will merge if CI passes.

[alimanfoo]

@sbalmer apologies I didn't know your full name to include in the release note, you're credited currently just as "sbalmer", happy to leave it that way or add your full name, whichever you prefer.

[jakirkham]

Thanks @sbalmer 😄

[sbalmer]

Thanks @alimanfoo, @jakirkham!

Now we can use mainline again :-)

[sbalmer]

When the chunk file is first removed before the new version
is moved into place, racing reads may encounter a missing chunk.

Using rename() or replace() without remove() avoids the issue
on Posix-Systems as the methods are atomic. The fallback of
remove() -> rename() is included for Windows pre Python 3.3.

Fixes#263

TODO:

• Add unit tests and/or doctests in docstrings
• Add docstrings and API docs for any new/modified user-facing classes and functions
• New/modified features documented in docs/tutorial.rst
• Changes documented in docs/release.rst
• Docs build locally (e.g., run tox -e docs)
• AppVeyor and Travis CI passes
• Test coverage is 100% (Coveralls passes)

[jakirkham]

So this is the case where we would want the pyosreplace package. It's a Windows only backport for os.replace on Python 2.

[alimanfoo]

Just to mention that there are some potential consequences from implementing this PR that we should be aware of, described in #328. I don't think that should stop us from moving ahead with this PR, using os.replace() is clearly a better thing to do under any circumstances, but just something to be aware of.

[sbalmer]

@jakirkham I've made a guess at the setup.py changes needed for pyosreplace. I couldn't get environmental markers to work. See previous commit. Could you please have a look at it? And is something else needed?

[jakirkham]

Think you had it basically right. Unfortunately there is some lack of clarity about when a colon or semicolon should be used. IIUC it should be a semicolon.

[jakirkham]

LGTM. Thanks for working on this @sbalmer.

Hopefully the comment above is useful. Am personally fine with how this is. Though if you want to use environment markers instead (or others prefer this), that could be also done, but don't personally think this is critical.

[alimanfoo]

Thanks @sbalmer. Could you add pyosreplace to requirements.txt and requirements_dev.txt, pinned to the latest version in the latter. I guess environment markers are needed in both.

[sbalmer]

@alimanfoopyosreplace is now in the requirement files too. I've made a guess at how the env markers should look. If I invert the predicates then pip install -r requirements.txt tries to install the package. Which means I have some confidence it should work.

[jakirkham]

Thanks @sbalmer. LGTM. 😄 Let's see what @alimanfoo thinks. 😉

[jakirkham]

I wonder if we can just drop the try...finally now.

[alimanfoo]

Many thanks @sbalmer, looks good. Could you add a release note to docs/release.rst.

[alimanfoo]

I wonder if we can just drop the try...finally now.

FWIW I imagine it could still be possible that some failure occurs during the attempt to write to the temporary file (e.g., device gets full or something like that), in which case an exception occurs before the value has been fully written, in which case we'd still want to clean up the temporary file.

[jakirkham]

Good point.

[alimanfoo]

I've resolved conflicts after merging #352 and added a release note. Will merge if CI passes.

[alimanfoo]

@sbalmer apologies I didn't know your full name to include in the release note, you're credited currently just as "sbalmer", happy to leave it that way or add your full name, whichever you prefer.

[jakirkham]

Thanks @sbalmer 😄

[sbalmer]

Thanks @alimanfoo, @jakirkham!

Now we can use mainline again :-)