Opened 12 years ago
Closed 11 years ago
#1316 closed defect (fixed)
[PATCH] Use inotify instead of FAM/Gamin on Linux
Reported by: | Philip Taylor | Owned by: | noKid |
---|---|---|---|
Priority: | Must Have | Milestone: | Alpha 12 |
Component: | Core engine | Keywords: | patch simple |
Cc: | Yves | Patch: |
Description
Currently we use the FAM API (usually implemented by Gamin, not the original FAM) to detect file and directory changes, so we can reload resources at run-time (useful for developers and modders).
Gamin doesn't seem to be actively maintained (last release was 3.5 years ago) and is a pretty heavyweight library for our needs. It'd probably be cleaner and more future-compatible to use the Linux kernel's inotify API directly, instead of using Gamin as an unnecessary wrapper around it. We don't need to support ancient pre-inotify Linux kernel versions, or non-Linux OSes.
I haven't looked into the details of inotify to see if it actually would work easily, or if there's any substantial trickiness required. Gamin has the advantage of generally working today, so it's probably not worth a huge amount of effort to change to inotify.
dir_watch_fam.cpp is the current implementation of the dir_watch API.
Attachments (6)
Change History (20)
comment:1 by , 12 years ago
Keywords: | review patch added |
---|---|
Milestone: | Backlog → Alpha 10 |
Summary: | Use inotify instead of FAM/Gamin on Linux → [PATCH] Use inotify instead of FAM/Gamin on Linux |
comment:2 by , 12 years ago
Keywords: | simple review patch → patch review simple |
---|---|
Owner: | set to |
Priority: | Should Have → Nice to Have |
Hey noKid. Thanks for your patch. Any chance you could test this on Mac OS X? Hotloading doesn't work on Mac, and it would be nice, if we're changing the system, to be compatible over all three OS's (Win/Linux/Mac).
Edit: Sorry, ignore my comment. I didn't notice your changes were for the linux folder only. Windows already works, and OSX will be taken care of in #514
I noticed too, it doesn't remove FAM/Gamin from any build file requirements? Are there any still linked after this patch change?
comment:3 by , 12 years ago
I find two tiny issues in the patch:
1.FAMCancelMonitor isn't replaced with inotify_rm_watch
2.inotifyfd is better defined as static
3.g_paths.push_back(*dirWatch); will trigger constructor/destructor of DirWatch, and may be not expected? My recommendation is to use std::map<int, PDirWatch> instead.
And after test on ubuntu 11.10,I find there seems to be a bug with inotify that each modification will invoke IN_MODIFY event twice.
comment:4 by , 12 years ago
To add to previous comments
- You can remove
CONFIG2_FAM
fromlib/config2.h
, similarly you no longer have to include that file (assuming the new implementation is fairly reliable and we don't need to disable it) without-fam
option and the related link commands can be removed frompremake4.lua
- Should rename
dir_watch_fam.cpp
todir_watch_inotify.cpp
or simplydir_watch.cpp
- Should omit the "removed stub" comment
comment:5 by , 12 years ago
Milestone: | Alpha 10 → Alpha 11 |
---|
comment:6 by , 12 years ago
Priority: | Nice to Have → Must Have |
---|
comment:7 by , 12 years ago
Elevating this because all the recent reports of the game freezing (usually in libfam) are troubling.
comment:8 by , 12 years ago
Hello,
I apologize for the lack of news. I read with attention your posts, and I provided a second version of the patch, with slight modifications (the ones listed by Alan).
I hope the Ubuntu problem is being resolved (the IN_MODIFY flag has been replaced by IN_CLOSE_WRITE).
About the game freezing, I haven't had any problems. I can't really do something without further infos.
You'll find in the attachements the patch for premake4.lua and lib/config2.h. I'm not yet familiar with the 0ad system, so I don't know if there are other files that need to be changed, and I'm sorry about that.
Also, I saw a recent ticket ( http://trac.wildfiregames.com/ticket/1420 ) about a problem that might occur with relative directories. I'm not 100 % sure about how it would reacts with inotify, but the modification is tiny, and not harmful (IMHO), so I put it in the 2nd patch.
comment:9 by , 12 years ago
Keywords: | review removed |
---|
Got around to reviewing the patch(es) :)
I resolved some conflicts with latest SVN, fixed a warning about signed/unsigned comparison, cleaned up most references to FAM, and attached a new combined patch.
There's an issue in how the inotify events are being handled. They actually have a watch bitmask unlike FAM which had a single code per FAMEvent
. We should change the field name in NotificationEvent
to mask
to reflect that, but more importantly treat it like a bitmask (not assuming they are mutually exclusive) since the case tests will fail when multiple bits flags are set. Maybe that means we need to push multiple notifications per event in some cases, or give one type priority over another?
Since you changed IN_MODIFY
to IN_CLOSE_WRITE
for inotify watches, I think it should be changed in dir_watch_Poll()
too.
Related: for some reason, hotloading is not working well in my Ubuntu 11.10 VM, even before your patch. I'm trying to confirm that GUI XML changes are picked up, but nothing happens. It works in Windows, maybe I can try booting into a real Ubuntu system and see if that makes a difference.
comment:10 by , 12 years ago
Milestone: | Alpha 11 → Alpha 12 |
---|
comment:11 by , 12 years ago
Cc: | added |
---|
comment:12 by , 12 years ago
There's something wrong or at least not optimal in inotify_deinit().
close(inotifyfd); causes select() to fail in inotify_event_loop because it closes the file descriptor that select is watching for changes. This will terminate the thread. Depending on how quickly the thread is terminated, sometimes pthread_cancel returns errorcode 3 (ESRCH No thread with the ID thread could be found). I'm not sure yet how to do it better, maybe I'll have a closer look later.
by , 12 years ago
Attachment: | dir_watch_fam.cpp.2.patch added |
---|
2nd version of the dir_watch_fam.cpp patch
comment:13 by , 11 years ago
I've figured out why I couldn't get hotloading to work on Ubuntu (see #1687). I tested your patch and it seems to work at least as well as the existing FAM implementation. I'm attaching a new combined patch for convenience.
by , 11 years ago
Attachment: | inotify2.patch added |
---|