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

Fix a bug in windows_file_system.cc for reading file #6805

Merged
merged 1 commit into from Jan 13, 2017

Conversation

Projects
None yet
4 participants
@meteorcloudy
Member

meteorcloudy commented Jan 12, 2017

When read_result is TRUE, result should be set to bytes_read.

https://msdn.microsoft.com/en-us/library/windows/desktop/ms686358(v=vs.85).aspx
"When a function is called to perform an overlapped operation, the operation might be completed before the function returns. When this happens, the results are handled as if the operation had been performed synchronously. If the operation was not completed, however, the function's return value is FALSE, and the GetLastError function returns ERROR_IO_PENDING."

Fix a bug in windows_file_system.cc for reading file
When read_result is TRUE, result should be set to bytes_read.

@googlebot googlebot added the cla: yes label Jan 12, 2017

@meteorcloudy

This comment has been minimized.

Show comment
Hide comment
@meteorcloudy

meteorcloudy Jan 12, 2017

Member

I found this bug when I was setting a TF job for windows on Bazel ci where python_op_gen_main failed to read hidden_ops.txt.

Member

meteorcloudy commented Jan 12, 2017

I found this bug when I was setting a TF job for windows on Bazel ci where python_op_gen_main failed to read hidden_ops.txt.

@drpngx

This comment has been minimized.

Show comment
Hide comment
@drpngx

drpngx Jan 12, 2017

Member

Is there another function that won't have a race condition?

Member

drpngx commented Jan 12, 2017

Is there another function that won't have a race condition?

@meteorcloudy

This comment has been minimized.

Show comment
Hide comment
@meteorcloudy

meteorcloudy Jan 13, 2017

Member

I am not sure. @mrry Do we use ::ReadFile for a reason? The problem is the code didn't deal with the situation when the function is performed as if it's synchronously. Is there a better solution?

Member

meteorcloudy commented Jan 13, 2017

I am not sure. @mrry Do we use ::ReadFile for a reason? The problem is the code didn't deal with the situation when the function is performed as if it's synchronously. Is there a better solution?

@mrry

This comment has been minimized.

Show comment
Hide comment
@mrry

mrry Jan 13, 2017

Contributor

@meteorcloudy I believe @vit-stepanovs contributed that particular piece of code, and I assume he used overlapped I/O here because it's the easiest way to read from a particular offset without a race condition if multiple threads are accessing the file. (@drpngx: I'm not sure what race condition you mean here. The return value is non-deterministic, sure, but as long as we handle it as Yun does here, I think it's threadsafe.)

The fix looks correct to me... I've had to fix similar issues with Windows API calls that way in the past, and I can't think of a more elegant solution.

Contributor

mrry commented Jan 13, 2017

@meteorcloudy I believe @vit-stepanovs contributed that particular piece of code, and I assume he used overlapped I/O here because it's the easiest way to read from a particular offset without a race condition if multiple threads are accessing the file. (@drpngx: I'm not sure what race condition you mean here. The return value is non-deterministic, sure, but as long as we handle it as Yun does here, I think it's threadsafe.)

The fix looks correct to me... I've had to fix similar issues with Windows API calls that way in the past, and I can't think of a more elegant solution.

@mrry

mrry approved these changes Jan 13, 2017

Thanks for fixing this!

@drpngx

This comment has been minimized.

Show comment
Hide comment
@drpngx

drpngx Jan 13, 2017

Member

I was thinking of GetLastError. Not a huge problem, but if other code thrashes errno then it could become a problem.

Member

drpngx commented Jan 13, 2017

I was thinking of GetLastError. Not a huge problem, but if other code thrashes errno then it could become a problem.

@mrry

This comment has been minimized.

Show comment
Hide comment
@mrry

mrry Jan 13, 2017

Contributor

Ah, I see what you mean. That does sound ominous, but according to the docs, GetLastError() returns errno for the calling thread, so we should be alright:

Retrieves the calling thread's last-error code value. The last-error code is maintained on a per-thread basis. Multiple threads do not overwrite each other's last-error code.

(How nice of the doc writer to state the property in triplicate!)

https://msdn.microsoft.com/en-us/library/windows/desktop/ms679360(v=vs.85).aspx

Contributor

mrry commented Jan 13, 2017

Ah, I see what you mean. That does sound ominous, but according to the docs, GetLastError() returns errno for the calling thread, so we should be alright:

Retrieves the calling thread's last-error code value. The last-error code is maintained on a per-thread basis. Multiple threads do not overwrite each other's last-error code.

(How nice of the doc writer to state the property in triplicate!)

https://msdn.microsoft.com/en-us/library/windows/desktop/ms679360(v=vs.85).aspx

@drpngx

This comment has been minimized.

Show comment
Hide comment
@drpngx

drpngx Jan 13, 2017

Member

Oh nice, good to know. Thanks!

Member

drpngx commented Jan 13, 2017

Oh nice, good to know. Thanks!

@drpngx drpngx merged commit 8f2579a into tensorflow:master Jan 13, 2017

9 of 11 checks passed

Linux CPU Tests (Python 3) FAILURE
Details
ci.tensorflow.org FAILURE
Details
Android Demo App SUCCESS
Details
Linux CPU Tests SUCCESS
Details
Linux CPU Tests CMAKE SUCCESS
Details
Linux CPU Tests Makefile SUCCESS
Details
Linux GPU SUCCESS
Details
MacOS CPU Tests SUCCESS
Details
Sanity Checks SUCCESS
Details
Windows Cmake Tests SUCCESS
Details
cla/google All necessary CLAs are signed

ppries added a commit to ppries/tensorflow that referenced this pull request Jan 16, 2017

Merge branch 'master' of https://github.com/tensorflow/tensorflow int…
…o slim-dynamic-flatten

* 'master' of https://github.com/tensorflow/tensorflow: (92 commits)
  Fix quantize_graph invocation in docs (tensorflow#6867)
  Make placement of constants follow consumers if they are all on the same device (tensorflow#6615)
  List of 2Ds -> 3D Tensor, seq2seq_loss (tensorflow#4382)
  The type of seq_len_max should be int64 (tensorflow#6859)
  remove unused flag (tensorflow#6857)
  Propagate seed in parallel_read to readers. Fixes tensorflow#6735 (tensorflow#6853)
  Have `write_graph` return the output path of file (tensorflow#6851)
  Disable building the MatrixDiag and OneHot kernels for GPU on Windows. (tensorflow#6822)
  Set NDK_ROOT in android_full.sh to fix Makefile build (tensorflow#6840)
  Update einsum to check whether uncompacted_shape has multiple None values (tensorflow#6830)
  Fix documentation sample code (tensorflow#6836)
  Enable bitcode for iOS builds by default (tensorflow#6825)
  Fix a bug in windows_file_system.cc for reading file (tensorflow#6805)
  Make tfdbg tests pass in Windows Bazel build (tensorflow#6821)
  Merge changes from github. Change: 144396000
  Update ops-related pbtxt files. Change: 144395527
  More new ops for speeding up input pipline: stage and unstage. Change: 144395110
  Fix the Windows cmake build. Change: 144394762
  Create a benchmark for resize_bicubic_op.
  Remove unnecessary control dependencies. Change: 144392019
  ...

@meteorcloudy meteorcloudy deleted the meteorcloudy:windows-patch-12 branch Mar 29, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment