jixxed / ed-odyssey-materials-helper

Elite Dangerous Odyssey Materials Helper
200 stars 32 forks source link

New log detection #34

Closed alexzk1 closed 2 years ago

alexzk1 commented 2 years ago

...it does not detect new game started if this program was already running (or at least proper name is not shown in status bar). I think that is because ....look on log:

{ "timestamp":"2021-07-26T19:43:11Z", "event":"Fileheader", "part":1, "language":"English\UK", "Odyssey":true, "gameversion":"4.0.0.600", "build":"r271793/r0 " } { "timestamp":"2021-07-26T19:44:15Z", "event":"Friends", "Status":"Online", "Name":"" } { "timestamp":"2021-07-26T19:47:42Z", "event":"Commander", "FID":"---", "Name":"---" }

On time-stamps. It is 4 seconds difference since 1st line and commander name happens in last line. However, this handler will be called on 1st line - once new file created - at best case, and that complex IF will reject event.

public void onCreated(final FileEvent event) {
                final File file = event.getFile();
                if (file.isFile()) {
                    if (file.getName().startsWith(AppConstants.JOURNAL_FILE_PREFIX) && hasFileHeader(file) && isOdysseyJournal(file) && hasCommanderHeader(file) && isSelectedCommander(file)) {
                        JournalWatcher.this.watchedFile = Optional.of(file);
                        fileCreatedProcessor.accept(file);
                    }
                }
            }

The event " public void onModified(final FileEvent event)" seems the same, except it calls different handler...didn't dig there too much.

Also FileProcessor.processJournal must be called from 1 thread with current implementation. Theoretically if game will restart fast and new file will be detected while that thing is running - results will be weird. However not possible to make in practice, you can try to break things by copying log files to folder while program runs.

jixxed commented 2 years ago

They used to be different, but now only onModified is needed. need to do some work on this class indeed. Statusbar shows latest file IF a message of it was processed, but that should be possible to do sooner. I should make it threadsafe even if it never happens in practice, don't know how this class will change in the future.

alexzk1 commented 2 years ago

Make this FileWatcher:: boolean poll = true; to be AtomicBoolean Because it writes in 2 places in stop() and during poll() which are different threads. Also inside thread, inside while, you must atomically read old + set new if old remains true, because while method poll() executes it could be called stop() which will set it false, but that false will be lost. Atomics allow to do read-set-check as single operation, guaranteed values will not be changed in between.

This is double problem here. boolean poll = true; Maybe cached on different processors so value of that will be not the same when 2 CPUs read it. To solve it you could use keyword volatile. Then 2nd problem - stop() writes false and poll() writes true -- false is lost. To solve it you could use synchronize keyword + dedicated Object for that. However, using atomics you may avoid both problems at once without delays for sync.

jixxed commented 2 years ago

Made some of the changes in the 1.34 release. Should be stable enough for now, but will investigate this a bit more the coming days. Bit hard to really focus now after my 2nd vaccination. Main features are in now, so time for some refactoring and testing.

alexzk1 commented 2 years ago

Made some of the changes in the 1.34 release. Should be stable enough for now, but will investigate this a bit more the coming days. Bit hard to really focus now after my 2nd vaccination. Main features are in now, so time for some refactoring and testing.

Yes, np. Just saying that code has fundamental error. Because that will be doing random errors at random times, I explained why. In short - you can't use normal variables if it is accessed by 2+ threads.

alexzk1 commented 2 years ago

You're getting close, proper would be

while (this.poll.getAndSet(pollEvents(watchService));

that will ensure if stop called prior pollEvents, it's false result will be noted.

P.S. don't forget to revise all threads u do there (if any).

P.P.S. They lowered Kit Flowler from 20 to 10 for unlock today.

alexzk1 commented 2 years ago

Yep, splitting into 2 vars was best solution. private boolean poll = true; now this can be local variable, to minimize side effects...and C++ style:

for (boolean poll = true;poll && this.run.get(); poll = pollEvents(watchService)) ;

jixxed commented 2 years ago

threading has been reworked