-
Notifications
You must be signed in to change notification settings - Fork 44
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
Use separate minimap temporary files per process when saving #21
base: master
Are you sure you want to change the base?
Conversation
Isn't it better to save minimap per player? |
Then you would have to discover everything from scratch every time you create new character. |
I don't think this solves anything. You would have to logout/close all clients at the exact same time when minimap is saved otherwise it saves minimap from first client, closes the file and then does it on the other client as well, unless it takes 10s for someone to save minimap 😅 If anything, minimap should be reworked into saving in chunks, that way if you have 2 players, one lets say in Venore and the other in Thais, they would be making changing to the minimap chunk that they were in instead of entire map. |
well thats actually the case Im solving. If 2 clients are logged out in the same time (ie. server save, server down, connection lost) and the map is pretty big (2MB is enough) it is possible that 2 clients will be writing to the file at the same time resulting in file corruption and losing the map as a whole. Thats what Im observing. I agree that this change is not a full solution - which would be merging 2 map files in case mentioned above, but thats not its goal. The goal is to avoid final map file corruption. |
It will save the map from the first client that executed save function. With your code it will be the opposite, the last client that executed the save function will overwrite every other client' minimap. There shouldn't be any corruption happening, opening file owned by a process will be simply prevented by OS. |
No, OS will not prevent from opening a file with write access from two different processes. At least not with basic open operations (without additional locking mechanisms present in some OSes). 2nd open operation will just clear the file content and lead to file corruption. Tested on Linux - maybe your OS is smarter, but I'd expect I encourage you to do a simple tests of running two dummy C/C++ programs writing to a file at the same time and then check how the file looks afterwards: |
Ah, so that's the issue. Yes, windows is blocking this, what I get is #include <iostream>
int main(int arc, const char *argv[])
{
FILE* file;
fopen_s(&file, "output.txt", "w");
if (file == NULL) {
perror("Error opening file");
return 1;
}
for (long long i = 0; i < 1000000; i++) {
fprintf(file, "%s", argv[1]);
}
std::string in;
std::cin >> in;
fclose(file);
printf("File closed.\n");
return 0;
} |
@Oen44 From my testing (Windows 10) it doesn't work. From what I remember in OTC there is a wrapper for OS functions that doesn't do it the correct way. It is called PHYSFS. This is an error that I get into the log file:
I don't know it the error msg is correct, but When I close local server, and have multiple OTC clients opened, the map is erased. PHYSFS_openWrite is the function that is used to create map file. @Jonekk your solution is only for Android build? |
I'm not sure how to respond. |
In short words: There is an issue in current code, that erase the minimap if you have more than one client opened and both are disconnected at the same time. I can confirm this issue on Windows build. |
My minimap was never erased, even when I got that warning message from using MC. It always saves the minimap from the client that didn't get the warning. As I said before, the fix is to rewrite minimaps so they are either saved per player name (meh solution) or make the minimap file save as parts (multiple chunks), which would be the best solution. |
Many players complained about this issue on my discord. |
Because you still lose the minimap from other clients. Right now it saves from client that closed FIRST, with this change it will save from client that closed LAST (assuming the saving was fast enough for all other clients to finish it, otherwise you still get an error). If you had 2 clients open, 1 char is in Thais, 2nd in Venore and they both start with empty minimap, when both clients close (they logout), currently you will only have minimap with Thais, with this PR changes, you would only have Venore. |
Without this change, I have 0 minimaps. |
No, the define youre looking at is |
@Jonekk I've made some testing using _getpid() on Windows (unistd.h do not exist on windows). If that happen I get this error in logs:
On other restarts it will eventualy remove this file, but if you close the client this file will stay there probably forever (till time OTC get the same process id and saves to same file). But I would for sure prefer to have "backup" minimap that takes some space on the drive than erase it entirely. But we would have to figure out way to make it works for Linux and Windows (probably some #ifdef code or maybe based on |
A quick and amateur solution would be to always create an execution file when the client opens, when the second client is opened, check if this file exists and if it does, do not allow the minimap file to be manipulated, when the first client is closed, delete this file. |
When there are two clients running, closing both of them at the same time can result in map loss on disk.
(both write to the same temporary file?). Solve that by using separate tmp files per PID. I assume any race on
rename
is not possible due to OS-specific mechanisms.