#3255 closed defect (fixed)
[PATCH] Prevent replay overwrites by using date and sequential ID
Reported by: | elexis | Owned by: | elexis |
---|---|---|---|
Priority: | Must Have | Milestone: | Alpha 20 |
Component: | Core engine | Keywords: | patch |
Cc: | Patch: |
Description (last modified by )
Currently the commands.txt files (and ooslog output) are created in a subdirectory of the sim-log directory.
The commands.txt file contains all commands sent by players and is therefore the key file when filing bug reports and replaying previous games visually with #9.
The problem I want to address here is that the subdirectory that contains the commands.txt file has a bad name. It uses the process ID and a sequential number. There are three problems with that:
- Old files will be overwritten.
- It is not easy to identify the correct directory when filing bugreports.
- The visual replay function #9 will be accessible from the main menu at some point. It should then display the correct date and time without relying on a fragile timestamp.
Deeper analysis:
1. Why files will be overwritten
Quoting from: http://en.wikipedia.org/wiki/Process_identifier#Unix-like
Process IDs are usually allocated on a sequential basis, beginning at 0 and rising to a maximum value which varies from system to system. Once this limit is reached, allocation restarts at 300 and again increases.
Therefore pIDs are not random but have a very limited keyspace (4 to 5 figures), which makes it very possible that old files will be overwritten. The sequential number that will be added to the PID will only be added if you play multiple games with the same instance of 0ad. If you reboot and get the same PID, then you overwrite the commands.txt file.
2. Identifying the correct directory
When people want to file a bugreport and the error appeared on the last game they played, then they only have to identify the file with the most recent filedate. Some have already problems with that (Mac users in particular, since that information isn't displayed by default). If the match was some time ago, the timestamp definitely helps identifying the correct file.
Open question: Should we make a subdirectory for the revision, so that the directory won't contain replay logs of different revisions? I.e. .../sim-log/alpha18/2015-05-20_03-38-38/commands.txt? No - that info should be saved to the commands.txt file.
Attachments (7)
Change History (30)
by , 9 years ago
Attachment: | t3255_use_datetime_in_simlog_directory.patch added |
---|
follow-up: 2 comment:1 by , 9 years ago
comment:2 by , 9 years ago
Replying to trompetin17:
if you have 3 or more 0ad open in your machine?
I think you mean point (1). It can happen that the commands.txt files are overwritten after rebooting when 0ad gets the same PID as some time earlier. The more files are in that directory, the more probable it becomes, since the PID only has 4 to 5 letters.
If the directory is empty, then the chance is 1/99999 = 0,001%. But if you test a lot then you have many files, for example 5000 on my machine. The probability that files will be overwritten is then 5000/99999 = 5% already. This means every 20th file will be overwritten.
It should be fixed for the other two reasons as well.
comment:3 by , 9 years ago
Keywords: | review removed |
---|
Broken in case of multiple instances, incomplete header changes, see discussion on irc.
by , 9 years ago
Attachment: | t3255_use_datetime_in_simlog_directory_v2.patch added |
---|
Now uses sim-log/YYYY-MM-DD_id/commands.txt
. Additional info like timestamp should be saved into the commands.txt file (see #3258).
comment:4 by , 9 years ago
Priority: | Should Have → Nice to Have |
---|---|
Summary: | [PATCH] Use date + time in sim-log directory instead of pid → [PATCH] Use date in commands.txt directory |
comment:5 by , 9 years ago
Description: | modified (diff) |
---|
comment:6 by , 9 years ago
TODO: CSimulation2Impl::DumpState
in Simulation2.cpp
also uses the old path for saving the oos_dump (-ooslog option).
TODO: CSimulation2Impl::ReportSerializationFailure
in Simulation2.cpp
should not use psLogDir() / "oos_log" but that directory above to save the oos_log, so that the file won't be overwritten with each oos error. This would allow one to create two oos_log files with two 0ad instances and diff them.
by , 9 years ago
Attachment: | t3255_use_timestamp_and_pid_for_oosdump_filename.patch added |
---|
If you run two instances of 0ad, rejoin with one player and if there is an oos, the oos_dump.txt files will have different filenames instead of being overwritten. This way you can diff the two oos-dumps. Should be included in the final patch of this ticket as well. The file should probably be moved to the sim-log subdirectory that also contains the commands.txt file.
comment:7 by , 9 years ago
Priority: | Nice to Have → Should Have |
---|
another TODO: the constructor of CReplayLogger creates an empty commands.txt and the parent directory as soon as one enters the gamesetup. If one leaves the gamesetup, the empty commands.txt file remains. Since the file is useless and replays are being listed / processed in #3258, the commands.txt file should be created only if the game actually starts. To accomplish that move the contents of the CReplayLogger constructor to the StartGame function.
comment:9 by , 9 years ago
comment:10 by , 9 years ago
Milestone: | Alpha 19 → Alpha 20 |
---|
by , 9 years ago
Attachment: | t3255_prevent_replay_overwrites_v3.patch added |
---|
comment:11 by , 9 years ago
Keywords: | review added |
---|---|
Milestone: | Alpha 20 → Alpha 19 |
Priority: | Should Have → Must Have |
Summary: | [PATCH] Use date in commands.txt directory → [PATCH] Prevent replay overwrites by using date and sequential ID |
Since now everyone has the ability to replay games, it would be quite sad if we let the game overwrite some replays every now and then.
(and yes, the patch works too if you start two games on one computer at the same millisecond.)
comment:12 by , 9 years ago
Owner: | set to |
---|
comment:13 by , 9 years ago
Milestone: | Alpha 19 → Alpha 20 |
---|
No, this is not urgent (it is easy to sort files by modification date, and overwriting won't happen frequently) and it is a change too important to get in after FF. So this stays in A20.
Actually, nothing at all should be brought back to A19 now. If they were pushed at some point there is a good reason.
by , 8 years ago
Attachment: | t3255_prevent_replay_overwrites_v4.patch added |
---|
comment:15 by , 8 years ago
Milestone: | Alpha 20 → Alpha 19 |
---|
The new patch addresses 3 issues:
- prevents accidental overwriting of replays
- uses
replays_v0.0.19/2015-11-02_007
instead ofsim_log/4863
- no more useless disk I/O (initially 1min+ loading time for outdated replays, #3433)
- user-friendly file-management
- defines the replay directory only in one place of the code
Notice the newly introduced replay root directory will be created in InitVfs()
of GameSetup.cpp
.
After commiting we need to replace sim_log
with replays_v0.0.19
in wiki:ReportingErrors, wiki:Debugging, wiki:EngineProfiling, stating that the last part is the version number of 0 A.D.
Replying to Itms:
overwriting won't happen frequently
Every replay one wants to keep needs to be renamed immediately after the match.
Replying to Itms:
not urgent
comment:16 by , 8 years ago
My comments:
I think this can be committed as is, but I really dislike the for loop on a conceptual basis. I think it'd be much better to just get all files in the directory, get the highest existing number, and add 1.
by , 8 years ago
Attachment: | t3255_prevent_replay_overwrites_v5.patch added |
---|
comment:19 by , 8 years ago
Updated patch:
- Moves replays to user documents (where also screenshots and savegames reside)
- Creates one directory per engine version to avoid mixing replay versions
- Supports alphanumerical sorting for 10k replays per day
- Prevents overwrites by using more than 4 digts for the index if needed
- Shows the directory of new replays to stdout
by , 8 years ago
Attachment: | t3255_prevent_replay_overwrites_v6.patch added |
---|
comment:20 by , 8 years ago
- remove the pid also from the serializationtest and ooslog directory path, since that is not really useful
- move the directory-index code to a shared function and use it for serializationtest and ooslog, to ease access for developers
comment:22 by , 8 years ago
Keywords: | review removed |
---|
if you have 3 or more 0ad open in your machine?