Winboard/Xboard patch request

[jhaglund]

Currently a game can be saved into a single file automatically, e.g., games.pgn.

However, when dozens are games are trying to be saved to a single file it will error.

lines 10775-10796 in backend.c

Code: Select all

/* Save the current game to the given file */
int
SaveGameToFile(filename, append)
     char *filename;
     int append;
{
    FILE *f;
    char buf[MSG_SIZ];

    if (strcmp(filename, "-") == 0) {
	return SaveGame(stdout, 0, NULL);
    } else {
	f = fopen(filename, append ? "a" : "w");
	if (f == NULL) {
	    snprintf(buf, sizeof(buf), _("Can't open \"%s\""), filename);
	    DisplayError(buf, errno);
	    return FALSE;
	} else {
	    return SaveGame(f, 0, NULL);
	}
    }
}

I propose 2 methods for a solution.

1) Store game into an error file, e.g., errno.pgn. This way we know it had an error, but still managed to store the game.

2) Create a multiple attempt to SaveGameToFile. Instead of just giving up right away and throwing a pop_up NULL error, why not try again after a little bit?

a) Random timer to save game. Pick a number between a range 1-10000 milliseconds, and save the game after Sleep(i).
b) Counter of attempts with a pause or Sleep(i) between attempts.

Like this:

Code: Select all

/* Save the current game to the given file */
int SaveGameToFile(filename, append)
     char *filename;
     int append;
{
     
FILE *f;
char buf[MSG_SIZ];
// Start of Joshua Haglund    
int i, min, max, range;
      time_t start,end;
      time (&start); 

   for(i = 15; i <= max;)
     {
      max = 3000;
      min = 1;
      range = max - min;
      i = i + 1;
      }
      i = 15+rand()% range;  
      Sleep(i);
  
    if (strcmp(filename, "-") == 0) {
       return SaveGame(stdout, 0, NULL);
    } else {
        i = 15+rand()% range;  
        Sleep(i);
// End of Joshua Haglund 
        f = fopen(filename, append ? "a" : "w");
	if (f == NULL) {
	    snprintf(buf, sizeof(buf), _("Can't open \"%s\""), filename);
        DisplayError(buf, errno);
	    return FALSE;
	} else {
	    return SaveGame(f, 0, NULL);
	}
    }
}

Early results seems to appear good. I do not have enough computing power to test extreme cases at the moment.

Thoughts
Joshua D. Haglund

[hgm]

What OS are you on? What you describe should _never_ happen. A call to fopen should only fail when there is something wrong with filename (e.g. non-existing directory, no permission to create a file in the directory, etc.). In that case retrying is guaranteed to have the same effect.

I also don't see what the 'dozens of games' have to do with it. Are you talking about concurrency here, where you have dozens of WinBoards running on the same machine in parallel, and they all try to save to the same file? I know Windows locks an open file for deleting. but I thought that overwriting, and certainly appending was allowed.

If some OS lock the file when one process is writing, it might be better to switch to a mode where shared access is allowed (e.g. fopen in "r+" mode), use (blocking) flock calls to protect th file contents from multiple simultaneous writers, and use fseek to position at the end before starting a write.

[jhaglund]

What OS are you on?

Windows Vista 32-bit

What you describe should _never_ happen.

It does happen. I've mentioned this before...

A call to fopen should only fail when there is something wrong with filename (e.g. non-existing directory, no permission to create a file in the directory, etc.).

It has to do with access because if one Winboard is trying to save using fopen(filename...) and another dozen are trying at the same time, then it will throw the error "Can't open ..." like in the code.

In that case retrying is guaranteed to have the same effect.

There's still a chance by with randomly selecting a time, or waiting your turn. It's just how long do you wait or attempts to try to save, before you return an error.

Are you talking about concurrency here, where you have dozens of WinBoards running on the same machine in parallel, and they all try to save to the same file?

Yes!

If some OS lock the file when one process is writing, it might be better to switch to a mode where shared access is allowed (e.g. fopen in "r+" mode), use (blocking) flock calls to protect th file contents from multiple simultaneous writers, and use fseek to position at the end before starting a write.

However you want to fix it is up to you. Since in the version I made it seems to work with the added fix... but there's more than one way to do it.

The ultimate test is to have 20-30 Winboards running and saving to a single file using "instanta-game" time controls, where you'll get games saving within ~ 7 milliseconds of each other.

[hgm]

OK, thanks to Fonzy I found out that this is a Windows peculiarity, (i.e. locking the file when another process has it open), and that MicroSoft supplies their own flavor of fopen to circumvent that, called _fsopen ("shared open").

So I guess I must replace the call to fopen here by one to my_fopen, which wuld have to be defined in the front-end. In XBoard my_fopen would then simply be a wrapper for fopen with the same arguments, but in WinBoard it would be a call to _fsopen in stead. The writing itself is already protected by flock calls, to prevent games from being intermingled. (In WinBoard 4.5-TM.)

I will work on it; I guess this same problem could occur for files that are read, like the -loadGameFile or -loadPositionFile? I'd better replace all fopen calls to my_fopen cals...

[hgm]

It remains weird that I cannot reproduce this problem on my Win XP. I made a test program to keep a file open for writing indefinitely:

Code: Select all

#include <stdio.h>
main()
{
 FILE *f=fopen("C:\\WinBoard-4.5-TM\\WinBoard\\test.pgn","a");
 fprintf(f,"{lock it}\n");
 fflush(f);
 getchar();
 fprintf(f,"{unlock it now}\n");
 fclose(f);
}

When I run it, and then open a WinBoard to let it savea game on the same file, there is no problem at all. The saved PGN nicely appears between the "lock" and "unlock" messages.

Is it a Windows user setting that determines if files are open for shared access by default?

[hgm]

Well, I uploaded a version now that uses _fsopen(filename, append ? "a" : "w", SH_DENYNO); in stead of then normal fopen. Please let me know if this makes the problem go away.

http://hgm.nubati.net/WinBoard-TM.zip

If it doesn't, I would be interested to know which reason exactly the error popup specifies for not being able to open the file.

[jhaglund]

Thanks for trying, but it's still there.
----------------------------------
Can't open "C:\chess.pgn":
The data is invalid.
----------------------------------

[hgm]

Strange error message, "the data is invalid". I could not find it in the list with MicroSoft errno codes. Perhaps I should modify the error popup such that it prints the actual error code as well as what it means. For all I know the "data is invalid" could mean that the error number is not known to the routine that must print the corresponding message.

[hgm]

I have now uploaded a version that prints the raw error number together with the message (same link). I could not test it because I could not think of an easy way togenerate errors. Could you try that for me? It should still give the same problem, but I would want to know what exactly that problem is.

The solution I am thinking of now is something like you propsed, except in a loop, so that it tries as often as needed. I don't want it to miss saving a file ever. But I don'twant it to get stuck in an infinite loop either when there is a genuine reason why the save does not work (like an invalid path name). So I want to loop back only when the error number for the failure that is given is equal to this mysterious case.

Using "sleep" here (or any form of delay) has the nasty effect that during it WinBoard will be completely unresponsive to external events.

[hgm]

I think this is error number 13. (I tried poppingup DisplayError with various error numbers, and for 13 I get something in Dutch that could be the same as "The data is invalid".) Which officially isknown as "Permission denied"

This is very problematic, though, as this error is what you are supposed to get ifyoudon't have write permission on the directory. Which would be a permanent condition, so I cannot loop on it.

I uploaded a version now that does upto 2 retries on a fail with errno == 13. This should alleviate the problem, although there is no guarantee it will always work. I guess it would be better to "stat" the file or diretory, to check what the permissions are, and if they are such that the operation inprinciple should suceed, keep trying as long as needed.