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

Generic backend issue excessive events #88

Closed
SpartanJ opened this issue Aug 6, 2015 · 13 comments
Closed

Generic backend issue excessive events #88

SpartanJ opened this issue Aug 6, 2015 · 13 comments
Labels
bug Something isn't working major

Comments

@SpartanJ
Copy link
Owner

SpartanJ commented Aug 6, 2015

Original report by Mihail Slobodyanuk (Bitbucket: mihail_slobodyanuk, ).


Hi, Martin!

I run into issue on Windows with generic backend.
Steps to reproduce:

  1. Establish watch to some folder e.g. c:\efsw\1
  2. Create in c:\efsw\1 new file.
  3. Delete c:\efsw\1 folder.
    Expected result: one delete for c:\efsw\1\file
    Actual result: infinity events loop about delete c:\efsw\1\file

Thanks!

@SpartanJ
Copy link
Owner Author

SpartanJ commented Aug 6, 2015

Original comment by Martín Lucas Golini (Bitbucket: SpartanJ, GitHub: SpartanJ).


Weird. I'll look into that.
Thanks!

@SpartanJ
Copy link
Owner Author

Original comment by Martín Lucas Golini (Bitbucket: SpartanJ, GitHub: SpartanJ).


I couldn't reproduce the bug, i've seen that you forked the repository and made some commits, it looks interesting, this is fixed in your fork?
I'm interested in merging your changes, the only change that i don't understand is the sys/inotify.h file added, i assume that it's because you're not using the fixed premake file ( see Issue #40 ) or the CMake file.

I'm really busy this month, but i'll keep an eye on your work.

Thanks for contributing for the library, i'm really happy to see some collaboration.

@SpartanJ
Copy link
Owner Author

Original comment by Martín Lucas Golini (Bitbucket: SpartanJ, GitHub: SpartanJ).


I'll close this issue because no response was given. But i'm still interested in knowing how to reproduce it. Mihail if you're still there please let me know.

Thanks

@SpartanJ
Copy link
Owner Author

Original comment by Martín Lucas Golini (Bitbucket: SpartanJ, GitHub: SpartanJ).


Fixed issue #48 in develop.

@SpartanJ
Copy link
Owner Author

Original comment by Martín Lucas Golini (Bitbucket: SpartanJ, GitHub: SpartanJ).


Fixed issue #48.

@SpartanJ
Copy link
Owner Author

Original comment by Mihail Slobodyanuk (Bitbucket: mihail_slobodyanuk, ).


I am sorry. I missed post from 2015-08-19.
I will check the issue again in next couple days.

About fork. I wanted finish QA then made merge request but the process delayed.
Today i pushed a couple additional fixes. We can discuss details then merge

@SpartanJ
Copy link
Owner Author

Original comment by Martín Lucas Golini (Bitbucket: SpartanJ, GitHub: SpartanJ).


Hi! Welcome back :)

I already have a branch with some of your changes incorporated but i used my own naming conventions, and i added made a couple of new changes.
The part i did not add are the changes in WatcherWin32, since at least for me doesn't seem to be working fine, i get intinite number of repeated messages for some reason, i did not look in detail your implementation, and i don't understand why you did those changes in a first place. Can you explain me what's the idea?

The new changes looks good, i'll add them later, except for the inotify.h header that i don't understand why is there.

Thank you very much for your collaboration.

Regards

@SpartanJ
Copy link
Owner Author

Original comment by Mihail Slobodyanuk (Bitbucket: mihail_slobodyanuk, ).


Hello, Martin!

Can you explain me what's the idea?

Are you meant https://bitbucket.org/mihail_slobodyanuk/efsw/commits/782d3f7f4a08c7015acd3f9f0b21e0145e1ec479 ?

My app create FileWatcher instance in one thread and destroys it in another thread. CancelIo() WinAPI require to be called in same thread as ReadDirectoryChangesW(). CancelIoEx() may help us but that one offered only since Windows Vista/Windows Server 2008 API. So i moved CancelIo() to same with ReadDirectoryChangesW() thread. All another changes in that commit are needed to proper thread synchronization.

... except for the inotify.h header that i don't understand why is there.

I moved inotify.h to another folder and include it via CFLAGS in outer make script. Yes i don't using premake every assembly. The main reason why i need own inotify.h is very outdated toolchains for some NAS. The linux kernel on that devices has inotify support, but GLIBC are not offer wrappers. I similar way the problem resolved in inotify-tools project (see inotify-nosys.h and system header detection in configure.ac )

Thanks. Mihail

@SpartanJ
Copy link
Owner Author

Original comment by Martín Lucas Golini (Bitbucket: SpartanJ, GitHub: SpartanJ).


My app create FileWatcher instance in one thread and destroys it in another thread. CancelIo() WinAPI require to be called in same thread as ReadDirectoryChangesW(). CancelIoEx() may help us but that one offered only since Windows Vista/Windows Server 2008 API. So i moved CancelIo() to same with ReadDirectoryChangesW() thread. All another changes in that commit are needed to proper thread synchronization.

Oh, now i get what you're saying. The problem is that there's already a patch for that problem, it was introduced later than your branch, i decided to basically drop Windows XP / 2000 support and just used CancelIoEx. I really don't see the point to support XP this days, by i know this is something that can be discussed, i just didn't want to go into the problem into implementing what you did. I would use your version if i find it stable so i can reintroduce Windows XP support, but with the bug i mentioned i can't, anyways i don't care much about supporting an old OS. Here's the fix: https://bitbucket.org/SpartanJ/efsw/commits/eb15d49e5c51

I moved inotify.h to another folder and include it via CFLAGS in outer make script. Yes i don't using premake every assembly. The main reason why i need own inotify.h is very outdated toolchains for some NAS. The linux kernel on that devices has inotify support, but GLIBC are not offer wrappers. I similar way the problem resolved in inotify-tools project (see inotify-nosys.h and system header detection in configure.ac )

Ok, inotify-nosys.h is already in the src/efsw/ subfolder. As i understand you wan't that file in another subfolder. I could just move it if that's inconvenient for you. The current implementation works like this:
When the make file is being created by premake4, inotify.h will be searched in the system, if is not found in the system, EFSW_INOTIFY_NOSYS will be added as a define to the file compilation. You should be able to do that in your configure.ac.

Thanks!

@SpartanJ
Copy link
Owner Author

Original comment by Mihail Slobodyanuk (Bitbucket: mihail_slobodyanuk, ).


Hi!
I had a little time and made test for the subj:

#!c++

#include <efsw/efsw.hpp>
#include <efsw/Thread.hpp>
#include <iostream>
#include <Windows.h>

using namespace std;

/// Processes a file action
class UpdateListener : public efsw::FileWatchListener {
public:

	UpdateListener() {
	}

	std::string getActionName(efsw::Action action) {
		switch (action) {
			case efsw::Actions::Add: return "Add";
			case efsw::Actions::Modified: return "Modified";
			case efsw::Actions::Delete: return "Delete";
			case efsw::Actions::Moved: return "Moved";
			default: return "Bad Action";
		}
	}

	void handleFileAction(efsw::WatchID watchid, const std::string& dir, const std::string& filename, efsw::Action action, std::string oldFilename = "") {
		std::cout << "DIR (" << dir + ") FILE (" + (oldFilename.empty() ? "" : "from file " + oldFilename + " to ") + filename + ") has event " << getActionName(action) << std::endl;
	}
};

string path = "C:\\efsw_test";
string file = path + "\\file";

void thread_proc() {
	CreateDirectory (path.c_str(), NULL);
	FILE* fd = fopen(file.c_str(), "w");
	char x = 'x';
	fwrite(&x, 1, 1, fd);
	fclose(fd);
	Sleep(2000);
	string cmd = "rmdir /s /q ";
	cmd+=path;
	system(cmd.c_str());
}

int main(int argc, char* argv[]) {
	UpdateListener * ul = new UpdateListener();
	efsw::Thread th(thread_proc);
	th.launch();
	efsw::FileWatcher * fileWatcher(new efsw::FileWatcher(true));
	efsw::WatchID err;
	if ((err = fileWatcher->addWatch(path, ul, true)) > 0) {
		fileWatcher->watch();
		std::cout << "Watching directory: " << path.c_str() << std::endl;
	} else {
		std::cout << "Error trying to watch directory: " << path.c_str() << std::endl;
		return 1;
	}
	Sleep(600*1000);
	delete fileWatcher;
	return 0;
}

It seems the bug still there. Here is my output:

Watching directory: C:\efsw_test
DIR (C:\efsw_test\) FILE (file) has event Modified
DIR (C:\efsw_test\) FILE (file) has event Delete
DIR (C:\efsw_test\) FILE (file) has event Delete
DIR (C:\efsw_test\) FILE (file) has event Delete
DIR (C:\efsw_test\) FILE (file) has event Delete
DIR (C:\efsw_test\) FILE (file) has event Delete
DIR (C:\efsw_test\) FILE (file) has event Delete
DIR (C:\efsw_test\) FILE (file) has event Delete
DIR (C:\efsw_test\) FILE (file) has event Delete

@SpartanJ
Copy link
Owner Author

Original comment by Mihail Slobodyanuk (Bitbucket: mihail_slobodyanuk, ).


Reproduced

@SpartanJ
Copy link
Owner Author

Original comment by Martín Lucas Golini (Bitbucket: SpartanJ, GitHub: SpartanJ).


I ran your test in default and develop branches and in both is working fine.

Are you sure you have the repo updated?

Here's my complete output for both cases:


Watching directory: C:\efsw_test

DIR (C:\efsw_test\) FILE (file) has event Delete

@SpartanJ
Copy link
Owner Author

Original comment by Mihail Slobodyanuk (Bitbucket: mihail_slobodyanuk, ).


Sorry. My bad somewhere. I re-updated repo and bug was gone.
Thank you!

@SpartanJ SpartanJ added major bug Something isn't working labels Jan 14, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working major
Projects
None yet
Development

No branches or pull requests

1 participant