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

Use separate minimap temporary files per process when saving #21

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

Jonekk
Copy link
Contributor

@Jonekk Jonekk commented Nov 7, 2024

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.

@punkice3407
Copy link

Isn't it better to save minimap per player?

@Oen44
Copy link
Collaborator

Oen44 commented Nov 7, 2024

Isn't it better to save minimap per player?

Then you would have to discover everything from scratch every time you create new character.

@Oen44
Copy link
Collaborator

Oen44 commented Nov 7, 2024

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.

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.

@Jonekk
Copy link
Contributor Author

Jonekk commented Nov 7, 2024

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.

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.

@Oen44
Copy link
Collaborator

Oen44 commented Nov 7, 2024

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.

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.

@Jonekk
Copy link
Contributor Author

Jonekk commented Nov 8, 2024

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 PHYSFS_openWrite to work consistently between OSes.

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:

Screenshot from 2024-11-08 12-47-28

@Oen44
Copy link
Collaborator

Oen44 commented Nov 8, 2024

Tested on Linux - maybe your OS is smarter

Ah, so that's the issue. Yes, windows is blocking this, what I get is Error opening file: Permission denied.
Added std::cin to wait for my input so I can run 2nd program while the first one still has file open (in case it writes faster than I can execute 2nd app).

#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;
}

@Feeltz
Copy link

Feeltz commented Nov 17, 2024

@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:

ERROR: failed to save OTMM minimap: rename: Proces nie może uzyskać dostępu do pliku, ponieważ jest on używany przez inny proces.: "C:\Users\jarek\AppData\Roaming\Trollheim Online\Trollheim Online DEV/minimap1098.otmm.tmp",

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.
My minimap is 2,3 MB size.

PHYSFS_openWrite is the function that is used to create map file.
obraz

@Jonekk your solution is only for Android build?

@Oen44
Copy link
Collaborator

Oen44 commented Nov 17, 2024

@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.

I'm not sure how to respond.

@Feeltz
Copy link

Feeltz commented Nov 17, 2024

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.

@Oen44
Copy link
Collaborator

Oen44 commented Nov 17, 2024

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.

@Feeltz
Copy link

Feeltz commented Nov 17, 2024

Many players complained about this issue on my discord.
But why saving as different named files, and then renaming it is a bad idea? It would make last client save the minimap.

@Oen44
Copy link
Collaborator

Oen44 commented Nov 17, 2024

But why saving as different named files, and then renaming it is a bad idea? It would make last client save the minimap.

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.

@Feeltz
Copy link

Feeltz commented Nov 18, 2024

Without this change, I have 0 minimaps.

@Jonekk
Copy link
Contributor Author

Jonekk commented Nov 18, 2024

@Jonekk your solution is only for Android build?

No, the define youre looking at is #ifndef ANDROID meaning "if NOT compiled for Android`

@Feeltz
Copy link

Feeltz commented Nov 18, 2024

@Oen44
obraz

as you can see, the minimap file size is decreasing. In this case like this, in other cases it is just pure 0 kb. after 1 restart. I don't know why, or what is causing it.

This is how minimap looks like after it went to 400 KB:
obraz

@Jonekk I can not compile it on Windows.
obraz

@Feeltz
Copy link

Feeltz commented Nov 18, 2024

@Jonekk I've made some testing using _getpid() on Windows (unistd.h do not exist on windows).
I've used 10 characters (10 OTC processes). Restarted server 20 times.
So far my minimap is working, not disappearing. However this approach gives other issue.
Sometimes, there is a *.tmp file saved. Like this:
obraz

If that happen I get this error in logs:

ERROR: failed to save OTMM minimap: rename: Odmowa dostępu.: "C:\Users\jarek\AppData\Roaming\Trollheim Online\Trollheim Online DEV/minimap1098.otmm.22504.tmp", "C:\Users\jarek\AppData\Roaming\Trollheim Online\Trollheim Online DEV/minimap1098.otmm"

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 std::string Application::getOs() method).

@lovinsfps
Copy link

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants