From de1a04038b45df4e25f81f8bef1008b5a978106d Mon Sep 17 00:00:00 2001 From: xomx Date: Tue, 10 Dec 2024 20:04:14 +0100 Subject: [PATCH] Fix network file wrong modification detection (regression from v8.7.1) - increased DEFAULT_MILLISEC constant from 1000 to 3000 (some slower networks obviously cannot respond within the previous 1sec response timeout). - the problematic situation was apparently made worse by the fact that the Buffer::checkFileState() did not issue one GetFileAttributesEx call but also an unnecessary second one (this was probably caused by an incorrect transcription of the previous version of this func for the new non-blocking thread method of calling the GetFileAttributesEx). - other problem was that the checking func did not check at all whether the threaded call to the GetFileAttributesEx succeeded or had to be forcibly terminated from the main app thread and expected successful calling only. - the nppLogNetworkDriveIssue logging has been enhanced. - added option to check possible WIN32API error code of the threaded GetFileAttributesEx call from the main N++ thread. Fix #15819, close #15936 --- PowerEditor/src/MISC/Common/Common.cpp | 14 +- PowerEditor/src/MISC/Common/Common.h | 6 +- PowerEditor/src/NppBigSwitch.cpp | 1 - PowerEditor/src/ScintillaComponent/Buffer.cpp | 160 ++++++++++++++---- 4 files changed, 138 insertions(+), 43 deletions(-) diff --git a/PowerEditor/src/MISC/Common/Common.cpp b/PowerEditor/src/MISC/Common/Common.cpp index f19c61741b8f..cb14c9e8136f 100644 --- a/PowerEditor/src/MISC/Common/Common.cpp +++ b/PowerEditor/src/MISC/Common/Common.cpp @@ -1751,7 +1751,7 @@ bool Version::isCompatibleTo(const Version& from, const Version& to) const } -#define DEFAULT_MILLISEC 1000 +#define DEFAULT_MILLISEC 3000 //---------------------------------------------------- @@ -1816,6 +1816,7 @@ struct GetAttrExParamResult wstring _filePath; WIN32_FILE_ATTRIBUTE_DATA _attributes{}; BOOL _result = FALSE; + DWORD _error = NO_ERROR; bool _isTimeoutReached = true; GetAttrExParamResult(wstring filePath): _filePath(filePath) { @@ -1826,12 +1827,16 @@ struct GetAttrExParamResult DWORD WINAPI getFileAttributesExWorker(void* data) { GetAttrExParamResult* inAndOut = static_cast(data); + ::SetLastError(NO_ERROR); inAndOut->_result = ::GetFileAttributesExW(inAndOut->_filePath.c_str(), GetFileExInfoStandard, &(inAndOut->_attributes)); + if (!(inAndOut->_result)) + inAndOut->_error = ::GetLastError(); inAndOut->_isTimeoutReached = false; return ERROR_SUCCESS; }; -BOOL getFileAttributesExWithTimeout(const wchar_t* filePath, WIN32_FILE_ATTRIBUTE_DATA* fileAttr, DWORD milliSec2wait, bool* isTimeoutReached) +BOOL getFileAttributesExWithTimeout(const wchar_t* filePath, WIN32_FILE_ATTRIBUTE_DATA* fileAttr, + DWORD milliSec2wait, bool* isTimeoutReached, DWORD* pdwWin32ApiError) { GetAttrExParamResult data(filePath); @@ -1855,13 +1860,16 @@ BOOL getFileAttributesExWithTimeout(const wchar_t* filePath, WIN32_FILE_ATTRIBUT ::TerminateThread(hThread, dwWaitStatus); break; } - CloseHandle(hThread); + ::CloseHandle(hThread); *fileAttr = data._attributes; if (isTimeoutReached != nullptr) *isTimeoutReached = data._isTimeoutReached; + if (pdwWin32ApiError != nullptr) + *pdwWin32ApiError = data._error; + return data._result; } diff --git a/PowerEditor/src/MISC/Common/Common.h b/PowerEditor/src/MISC/Common/Common.h index cc7c608c0366..3d3548030933 100644 --- a/PowerEditor/src/MISC/Common/Common.h +++ b/PowerEditor/src/MISC/Common/Common.h @@ -280,8 +280,10 @@ class Version final }; -BOOL getDiskFreeSpaceWithTimeout(const wchar_t* dirPath, ULARGE_INTEGER* freeBytesForUser, DWORD milliSec2wait = 0, bool* isTimeoutReached = nullptr); -BOOL getFileAttributesExWithTimeout(const wchar_t* filePath, WIN32_FILE_ATTRIBUTE_DATA* fileAttr, DWORD milliSec2wait = 0, bool* isTimeoutReached = nullptr); +BOOL getDiskFreeSpaceWithTimeout(const wchar_t* dirPath, ULARGE_INTEGER* freeBytesForUser, + DWORD milliSec2wait = 0, bool* isTimeoutReached = nullptr); +BOOL getFileAttributesExWithTimeout(const wchar_t* filePath, WIN32_FILE_ATTRIBUTE_DATA* fileAttr, + DWORD milliSec2wait = 0, bool* isTimeoutReached = nullptr, DWORD* pdwWin32ApiError = nullptr); bool doesFileExist(const wchar_t* filePath, DWORD milliSec2wait = 0, bool* isTimeoutReached = nullptr); bool doesDirectoryExist(const wchar_t* dirPath, DWORD milliSec2wait = 0, bool* isTimeoutReached = nullptr); diff --git a/PowerEditor/src/NppBigSwitch.cpp b/PowerEditor/src/NppBigSwitch.cpp index 08a8ddeb3759..85f44b0b8236 100644 --- a/PowerEditor/src/NppBigSwitch.cpp +++ b/PowerEditor/src/NppBigSwitch.cpp @@ -2191,7 +2191,6 @@ LRESULT Notepad_plus::process(HWND hwnd, UINT message, WPARAM wParam, LPARAM lPa if (nppgui._fileAutoDetection != cdDisabled) { bool bCheckOnlyCurrentBuffer = (nppgui._fileAutoDetection & cdEnabledNew) ? true : false; - checkModifiedDocument(bCheckOnlyCurrentBuffer); return TRUE; } diff --git a/PowerEditor/src/ScintillaComponent/Buffer.cpp b/PowerEditor/src/ScintillaComponent/Buffer.cpp index a57df53627f9..9196f6a1a81b 100644 --- a/PowerEditor/src/ScintillaComponent/Buffer.cpp +++ b/PowerEditor/src/ScintillaComponent/Buffer.cpp @@ -142,12 +142,49 @@ void Buffer::setLangType(LangType lang, const wchar_t* userLangName) void Buffer::updateTimeStamp() { FILETIME timeStampLive {}; + WIN32_FILE_ATTRIBUTE_DATA attributes{}; attributes.dwFileAttributes = INVALID_FILE_ATTRIBUTES; - if (getFileAttributesExWithTimeout(_fullPathName.c_str(), &attributes)) + bool bWorkerThreadTerminated = true; + DWORD dwWin32ApiError = NO_ERROR; + BOOL bGetFileAttributesExSucceeded = getFileAttributesExWithTimeout(_fullPathName.c_str(), &attributes, 0, &bWorkerThreadTerminated, &dwWin32ApiError); + if (bGetFileAttributesExSucceeded && (attributes.dwFileAttributes != INVALID_FILE_ATTRIBUTES) && !(attributes.dwFileAttributes & FILE_ATTRIBUTE_DIRECTORY)) { timeStampLive = attributes.ftLastWriteTime; } + else + { + if (_currentStatus != DOC_UNNAMED) + { + NppParameters& nppParam = NppParameters::getInstance(); + if (nppParam.doNppLogNetworkDriveIssue()) + { + wstring issueFn = nppLogNetworkDriveIssue; + issueFn += L".log"; + wstring nppIssueLog = nppParam.getUserPath(); + pathAppend(nppIssueLog, issueFn); + std::string msg = wstring2string(_fullPathName, CP_UTF8); + msg += " in Buffer::updateTimeStamp(), getFileAttributesExWithTimeout returned "; + if (bGetFileAttributesExSucceeded) + msg += "TRUE"; + else + msg += "FALSE"; + if (bWorkerThreadTerminated) + { + msg += ", its worker thread had to be forcefully terminated due to timeout reached!"; + } + else + { + msg += ", its worker thread finished successfully within the timeout given, "; + if (attributes.dwFileAttributes == INVALID_FILE_ATTRIBUTES) + msg += "dwFileAttributes == INVALID_FILE_ATTRIBUTES ! (WIN32API Error Code: " + std::to_string(dwWin32ApiError) + ")"; + else + msg += "dwFileAttributes has the FILE_ATTRIBUTE_DIRECTORY flag set!"; + } + writeLog(nppIssueLog.c_str(), msg.c_str()); + } + } + } LONG res = CompareFileTime(&_timeStamp, &timeStampLive); if (res == -1 || res == 1) @@ -157,19 +194,28 @@ void Buffer::updateTimeStamp() // It can happen when user copies a backup of editing file somewhere-else firstly, then modifies the editing file in Notepad++ and saves it. // Now user copies the backup back to erase the modified editing file outside Notepad++ (via Explorer). { - if (res == 1) + NppParameters& nppParam = NppParameters::getInstance(); + if (nppParam.doNppLogNetworkDriveIssue()) { - NppParameters& nppParam = NppParameters::getInstance(); - if (nppParam.doNppLogNetworkDriveIssue()) + char buf[1024]{}; + if (res == 1) + { + sprintf_s(buf, _countof(buf) - 1, " in updateTimeStamp(): timeStampLive (%lu/%lu) < _timeStamp (%lu/%lu)", + timeStampLive.dwLowDateTime, timeStampLive.dwHighDateTime, _timeStamp.dwLowDateTime, _timeStamp.dwHighDateTime); + } + else + { + // usual case, uncomment if needed to log too + //sprintf_s(buf, _countof(buf) - 1, " in updateTimeStamp(): timeStampLive (%lu/%lu) > _timeStamp (%lu/%lu)", + // timeStampLive.dwLowDateTime, timeStampLive.dwHighDateTime, _timeStamp.dwLowDateTime, _timeStamp.dwHighDateTime); + } + if (buf[0] != '\0') { wstring issueFn = nppLogNetworkDriveIssue; issueFn += L".log"; wstring nppIssueLog = nppParam.getUserPath(); pathAppend(nppIssueLog, issueFn); - std::string msg = wstring2string(_fullPathName, CP_UTF8); - char buf[1024]; - sprintf(buf, " in updateTimeStamp(): timeStampLive (%lu/%lu) < _timeStamp (%lu/%lu)", timeStampLive.dwLowDateTime, timeStampLive.dwHighDateTime, _timeStamp.dwLowDateTime, _timeStamp.dwHighDateTime); msg += buf; writeLog(nppIssueLog.c_str(), msg.c_str()); } @@ -260,10 +306,14 @@ bool Buffer::checkFileState() // returns true if the status has been changed (it if (_currentStatus == DOC_UNNAMED || isMonitoringOn()) return false; + NppParameters& nppParam = NppParameters::getInstance(); + WIN32_FILE_ATTRIBUTE_DATA attributes{}; attributes.dwFileAttributes = INVALID_FILE_ATTRIBUTES; - NppParameters& nppParam = NppParameters::getInstance(); - bool fileExists = doesFileExist(_fullPathName.c_str()); + bool bWorkerThreadTerminated = true; + DWORD dwWin32ApiError = NO_ERROR; + BOOL bGetFileAttributesExSucceeded = getFileAttributesExWithTimeout(_fullPathName.c_str(), &attributes, 0, &bWorkerThreadTerminated, &dwWin32ApiError); + bool fileExists = (bGetFileAttributesExSucceeded && (attributes.dwFileAttributes != INVALID_FILE_ATTRIBUTES) && !(attributes.dwFileAttributes & FILE_ATTRIBUTE_DIRECTORY)); #ifndef _WIN64 bool isWow64Off = false; @@ -272,10 +322,42 @@ bool Buffer::checkFileState() // returns true if the status has been changed (it nppParam.safeWow64EnableWow64FsRedirection(FALSE); isWow64Off = true; - fileExists = doesFileExist(_fullPathName.c_str()); + bGetFileAttributesExSucceeded = getFileAttributesExWithTimeout(_fullPathName.c_str(), &attributes, 0, &bWorkerThreadTerminated, &dwWin32ApiError); + fileExists = (bGetFileAttributesExSucceeded && (attributes.dwFileAttributes != INVALID_FILE_ATTRIBUTES) && !(attributes.dwFileAttributes & FILE_ATTRIBUTE_DIRECTORY)); } #endif + if (!fileExists && nppParam.doNppLogNetworkDriveIssue()) + { + wstring issueFn = nppLogNetworkDriveIssue; + issueFn += L".log"; + wstring nppIssueLog = nppParam.getUserPath(); + pathAppend(nppIssueLog, issueFn); + std::string msg = wstring2string(_fullPathName, CP_UTF8); + msg += " in Buffer::checkFileState(), getFileAttributesExWithTimeout returned "; + if (bGetFileAttributesExSucceeded) + { + msg += "TRUE"; + } + else + { + msg += "FALSE"; + } + if (bWorkerThreadTerminated) + { + msg += ", its worker thread had to be forcefully terminated due to timeout reached!"; + } + else + { + msg += ", its worker thread finished successfully within the timeout given, "; + if (attributes.dwFileAttributes == INVALID_FILE_ATTRIBUTES) + msg += "dwFileAttributes == INVALID_FILE_ATTRIBUTES ! (WIN32API Error Code: " + std::to_string(dwWin32ApiError) + ")"; + else + msg += "dwFileAttributes has the FILE_ATTRIBUTE_DIRECTORY flag set!"; + } + writeLog(nppIssueLog.c_str(), msg.c_str()); + } + bool isOK = false; if (_currentStatus == DOC_INACCESSIBLE && !fileExists) //document is absent on its first load - we set readonly and not dirty, and make it be as document which has been deleted { @@ -291,29 +373,25 @@ bool Buffer::checkFileState() // returns true if the status has been changed (it { _currentStatus = DOC_DELETED; _isFileReadOnly = false; - _isDirty = true; //dirty sicne no match with filesystem + _isDirty = true; //dirty since no match with filesystem _timeStamp = {}; doNotify(BufferChangeStatus | BufferChangeReadonly | BufferChangeTimestamp); isOK = true; } else if (_currentStatus == DOC_DELETED && fileExists) //document has returned from its grave { - if (GetFileAttributesEx(_fullPathName.c_str(), GetFileExInfoStandard, &attributes) != 0) // fileExists so it's safe to call GetFileAttributesEx directly + // fileExists==true here means that we can use the previously obtained attributes safely + _isFileReadOnly = attributes.dwFileAttributes & FILE_ATTRIBUTE_READONLY; + _currentStatus = DOC_MODIFIED; + _timeStamp = attributes.ftLastWriteTime; + if (_reloadFromDiskRequestGuard.try_lock()) { - _isFileReadOnly = attributes.dwFileAttributes & FILE_ATTRIBUTE_READONLY; - - _currentStatus = DOC_MODIFIED; - _timeStamp = attributes.ftLastWriteTime; - - if (_reloadFromDiskRequestGuard.try_lock()) - { - doNotify(BufferChangeStatus | BufferChangeReadonly | BufferChangeTimestamp); - _reloadFromDiskRequestGuard.unlock(); - } - isOK = true; + doNotify(BufferChangeStatus | BufferChangeReadonly | BufferChangeTimestamp); + _reloadFromDiskRequestGuard.unlock(); } + isOK = true; } - else if (getFileAttributesExWithTimeout(_fullPathName.c_str(), &attributes)) + else if (bGetFileAttributesExSucceeded) { int mask = 0; //status always 'changes', even if from modified to modified bool isFileReadOnly = attributes.dwFileAttributes & FILE_ATTRIBUTE_READONLY; @@ -324,30 +402,39 @@ bool Buffer::checkFileState() // returns true if the status has been changed (it } LONG res = CompareFileTime(&_timeStamp, &attributes.ftLastWriteTime); - if (res == -1 || res == 1) - // (res == -1) => attributes.ftLastWriteTime is later, it means the file has been modified outside of Notepad++ - usual case - // - // (res == 1) => The timestamp get directly from the file on disk is earlier than buffer's timestamp - unusual case - // It can happen when user copies a backup of editing file somewhere-else firstly, then modifies the editing file in Notepad++ and saves it. - // Now user copies the backup back to erase the modified editing file outside Notepad++ (via Explorer). + // (res == -1) => attributes.ftLastWriteTime is later, it means the file has been modified outside of Notepad++ - usual case + // + // (res == 1) => The timestamp get directly from the file on disk is earlier than buffer's timestamp - unusual case + // It can happen when user copies a backup of editing file somewhere-else firstly, then modifies the editing file in Notepad++ and saves it. + // Now user copies the backup back to erase the modified editing file outside Notepad++ (via Explorer). { - if (res == 1) + if (nppParam.doNppLogNetworkDriveIssue()) { - if (nppParam.doNppLogNetworkDriveIssue()) + char buf[1024]{}; + if (res == 1) + { + sprintf_s(buf, _countof(buf) - 1, " in checkFileState(): attributes.ftLastWriteTime (%lu/%lu) < _timeStamp (%lu/%lu)", + attributes.ftLastWriteTime.dwLowDateTime, attributes.ftLastWriteTime.dwHighDateTime, _timeStamp.dwLowDateTime, _timeStamp.dwHighDateTime); + } + else + { + // usual state after a file modification, uncomment if needed to log too + //sprintf_s(buf, _countof(buf) - 1, " in checkFileState(): attributes.ftLastWriteTime (%lu/%lu) > _timeStamp (%lu/%lu)", + // attributes.ftLastWriteTime.dwLowDateTime, attributes.ftLastWriteTime.dwHighDateTime, _timeStamp.dwLowDateTime, _timeStamp.dwHighDateTime); + } + if (buf[0] != '\0') { wstring issueFn = nppLogNetworkDriveIssue; issueFn += L".log"; wstring nppIssueLog = nppParam.getUserPath(); pathAppend(nppIssueLog, issueFn); - std::string msg = wstring2string(_fullPathName, CP_UTF8); - char buf[1024]; - sprintf(buf, " in checkFileState(): attributes.ftLastWriteTime (%lu/%lu) < _timeStamp (%lu/%lu)", attributes.ftLastWriteTime.dwLowDateTime, attributes.ftLastWriteTime.dwHighDateTime, _timeStamp.dwLowDateTime, _timeStamp.dwHighDateTime); msg += buf; writeLog(nppIssueLog.c_str(), msg.c_str()); } } + _timeStamp = attributes.ftLastWriteTime; mask |= BufferChangeTimestamp; _currentStatus = DOC_MODIFIED; @@ -360,9 +447,7 @@ bool Buffer::checkFileState() // returns true if the status has been changed (it if (_reloadFromDiskRequestGuard.try_lock()) { doNotify(mask); - _reloadFromDiskRequestGuard.unlock(); - return true; } } @@ -376,6 +461,7 @@ bool Buffer::checkFileState() // returns true if the status has been changed (it nppParam.safeWow64EnableWow64FsRedirection(TRUE); } #endif + return isOK; }