Fix of not picking file modifications. #12353

mfatekho · 2016-04-08T16:41:35Z

Inapproriate use of win api in fs_even_node module. In fsevents_win.cpp there are a thread that monitors file system events queue and post messages to uv library. Proposed solution is to use extra bool variable in THREADINFO structure to control thread exit state
@busykai

Inapproriate use of win api in fs_even_node module. In fsevents_win.cpp there are a thread that monitors file system events queue and post messages to uv library. Proposed solution is to use extra bool variable in THREADINFO structure to control thread exit state

busykai · 2016-04-11T20:29:20Z

src/filesystem/impls/appshell/node/node_modules/fsevents_win/fsevents_win.cpp

@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2014 - present Adobe Systems Incorporated. All rights reserved.


~~This change, the base of your commit, seems to be a left-over of the intermediate work. Could you please rebase the commit on top of adobe/master?~~

EDIT: this is an incorrect comment, my bad.

busykai · 2016-04-11T20:59:46Z

@peterflynn, @bchintx, @abose: we were able to reproduce the filewatchers going offline. we expect this is supposed to fix #7638 and related issues. The issue occurs when some other tools interferes with the same file(s) causing "spurious" messages which result in this condition resolves to FALSE and causes the filewatchers to quit abruptly.

The fix consists of keeping an internal flag of whether the filewatchers should quit instead of relying on specific parameters of the "terminate" message.

busykai · 2016-04-11T21:01:28Z

src/filesystem/impls/appshell/node/node_modules/fsevents_win/fsevents_win.cpp

@@ -337,7 +339,7 @@ DWORD WINAPI NodeFSEvents::Run(LPVOID lpData)
 	{
 		// iterate watching for directory changes until we're signalled to stop
 		BOOL bContinue = TRUE;


This variable should be taken off entirely. Since you removed the code at lines 386-390, bContinue never changes the value and having it around is excessive and not needed.

According to changes now bContinue reflects status from winapi call at line 346 ReadDirectoryChangesW, in some situations that function may fail and return false according to MSDN, that leads to looper exit. Do you propose to skip error handling for that function and continue watching thread as well?

Nevermind, I simply missed that line. You're right, the logic's good.

@mfatekho I think there's still a logic error here. If the ReadDirectoryChangesW() call below fails, then bContinue is assigned the value of FALSE. As a result, on the next iteration of the loop, the while condition below in line 342 will still be false, and we'll still get kicked out of the loop.

I'd partially agree with @busykai 's comment above, and propose removing bContinue from the while loop condition expression in line 342. In fact, you might as well get rid of bContinue variable altogether and just move the ReadDirectoryChangesW() call inside the if condition in line 351.

Hi @bchintx there are several points with removing bContinue and leaving the rest untouched.

on line 247 there are FILE_FLAG_OVERLAPPED is set meaning ReadDirectoryChangesSW is working in async mode.

MSDN says that for asynchronous mode returning value is a result of task submission, false means we run out of some resources, handles, stack, ram, etc.

Submitting new requests, e.g. calling ReadDirectoryChangesSW in that situation (if that ever happen i believe in low resource condition there will be lots of problems) makes situation even worse: I'm not sure whether that function will issue SwitchToThread call, if not we will have one cpu core load spot until low resource condition go away.

Further reading of possible error codes reported by ReadDirectoryChangesW() makes me think they all are not recoverable, so no point to keep that loop

I just wanted to say that if we simply ignore that error we have to think about possible short loop that burns CPU, and at least add SwitchToThread(), Sleep(0), etc, if we decided to not abort watcher thread.

Let's just get rid of bContinue it bothers me (both its name and its use in the while condition). Just like @bchintx pointed out, let's wrap the ReadDirectory... call in an if, this way @evgen48's concern of not handling the error condition will be addressed too.

busykai · 2016-04-13T15:57:49Z

LGTM. Will merge soon after the copyright is fixed.

swmitra · 2016-04-14T08:13:46Z

LGTM 👍

MiguelCastillo · 2016-04-14T13:36:06Z

This looks good to me. Totally got PTSD from old C++ days... :D

ingorichter · 2016-04-14T21:40:05Z

Looks like a simple fix to get rid of the problem.

bchintx · 2016-04-14T23:14:19Z

Please don't merge quite yet. Please see my comment inline above, as I think there's still a logic error that will prevent the fix from working as expected.

ingorichter · 2016-04-14T23:17:56Z

@bchintx I was thinking about getting rid of bContinue, but you said it way better than I could have done this. ;-)

mfatekho · 2016-04-27T13:14:58Z

Hi guys,
Refactored code a little due to your recommendations. As you suggested, I have removed bContinue variable and wrapped ReadDirectoryChangesW to if.

busykai · 2016-04-27T19:53:37Z

src/filesystem/impls/appshell/node/node_modules/fsevents_win/fsevents_win.cpp

-			{
+				&dwBytesReturned, lpThreadInfo->m_lpOverlapped, NULL)) {
+				break;
+			}
 				// wait for a signal


Lines 351-358 can be indented left now.

ingorichter · 2016-05-04T21:41:17Z

Poking @bchintx to have another look.

bchintx · 2016-05-04T23:46:48Z

Looks good to me. Merging!

Nice work @mfatekho!

busykai · 2016-05-05T15:55:30Z

Yay! Thanks @mfatekho and @evgen48 for root-causing and fixing this!

MegaArman · 2020-09-23T03:21:26Z

Just trying out brackets for the first time today. Here's my trouble scenario:
-have some files open
-git checkout in my terminal
-the files are not reloaded automatically nor am I prompted to reload them
-Write some code
-Hit ctrl-s and now I am given a dialogue window if I want to overwrite what's on disc. This is an issue because now I have to figure out if my changes to the file are actually OK

In the Geany editor under Edit->Preferences->Files-> Disc check timeout, you can set how often the disk is checked and it will prompt you to reload the file if changes are detected

Is there something like that for Brackets?

busykai reviewed Apr 11, 2016
View reviewed changes

mfatekho added 2 commits April 15, 2016 11:37

Innapropriate copyright fixed.

6b6d290

Minor refactoring

bea5f1c

busykai reviewed Apr 27, 2016
View reviewed changes

Indention fixed

b0067fe

bchintx merged commit a78983d into adobe:master May 4, 2016

marcelgerber mentioned this pull request May 15, 2016

Brackets sometimes doesn't pick up file modifications #7638

Closed

core-ai-bot mentioned this pull request Aug 30, 2021

[CLOSED] Fix of not picking file modifications. brackets-archive/bracketsIssues#10588

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix of not picking file modifications. #12353

Fix of not picking file modifications. #12353

mfatekho commented Apr 8, 2016

busykai Apr 11, 2016

busykai commented Apr 11, 2016

busykai Apr 11, 2016

evgen48 Apr 13, 2016

busykai Apr 13, 2016

bchintx Apr 14, 2016 •

edited

Loading

evgen48 Apr 15, 2016

busykai Apr 15, 2016 •

edited

Loading

busykai commented Apr 13, 2016

swmitra commented Apr 14, 2016

MiguelCastillo commented Apr 14, 2016

ingorichter commented Apr 14, 2016

bchintx commented Apr 14, 2016

ingorichter commented Apr 14, 2016

mfatekho commented Apr 27, 2016

busykai Apr 27, 2016

ingorichter commented May 4, 2016

bchintx commented May 4, 2016 •

edited

Loading

busykai commented May 5, 2016

MegaArman commented Sep 23, 2020 •

edited

Loading

		@@ -1,5 +1,5 @@
		/*
		* Copyright (c) 2014 - present Adobe Systems Incorporated. All rights reserved.

Fix of not picking file modifications. #12353

Fix of not picking file modifications. #12353

Conversation

mfatekho commented Apr 8, 2016

busykai Apr 11, 2016

Choose a reason for hiding this comment

busykai commented Apr 11, 2016

busykai Apr 11, 2016

Choose a reason for hiding this comment

evgen48 Apr 13, 2016

Choose a reason for hiding this comment

busykai Apr 13, 2016

Choose a reason for hiding this comment

bchintx Apr 14, 2016 • edited Loading

Choose a reason for hiding this comment

evgen48 Apr 15, 2016

Choose a reason for hiding this comment

busykai Apr 15, 2016 • edited Loading

Choose a reason for hiding this comment

busykai commented Apr 13, 2016

swmitra commented Apr 14, 2016

MiguelCastillo commented Apr 14, 2016

ingorichter commented Apr 14, 2016

bchintx commented Apr 14, 2016

ingorichter commented Apr 14, 2016

mfatekho commented Apr 27, 2016

busykai Apr 27, 2016

Choose a reason for hiding this comment

ingorichter commented May 4, 2016

bchintx commented May 4, 2016 • edited Loading

busykai commented May 5, 2016

MegaArman commented Sep 23, 2020 • edited Loading

bchintx Apr 14, 2016 •

edited

Loading

busykai Apr 15, 2016 •

edited

Loading

bchintx commented May 4, 2016 •

edited

Loading

MegaArman commented Sep 23, 2020 •

edited

Loading