There is the following code:

public AttachedFile SaveAttachment(NewAttachment attachment) { var tmpFile = Path.GetTempFileName(); try { using (var f = File.Open(tmpFile, FileMode.Append)) { attachment.Stream.CopyTo(f); } string filePath = string.Empty; string dst = string.Empty; do { filePath = generatePathForAttachment(); dst = Path.Combine(archiveDirectory, filePath); } while (File.Exists(Path.Combine(archiveDirectory, filePath))); Directory.CreateDirectory(Path.GetDirectoryName(dst)); var attachedFile = new AttachedFile { OriginalTitle = attachment.OriginalTitle, Path = filePath, Size = attachment.Stream.Length, AuthorId = attachment.AuthorId, Created = DateTime.Now }; File.Move(tmpFile, dst); _context.Set<AttachedFile>().Add(attachedFile); _context.SaveChanges(); return attachedFile; } catch (Exception) { throw; } }

its purpose is to save the file on the server, the code is working.

Help to modify this code to reduce the consequences of the occurrence of Exception to a minimum, i.e. I need to be done either all or nothing.

My imagination was enough only for wrapping each problem area in try , i.e. something like:

 try { if (!Directory.Exists(Path.GetDirectoryName(dst))) Directory.CreateDirectory(Path.GetDirectoryName(dst)); } catch(Exception) { //обработка исключения } try { if (File.Exists(dst)) File.Move(dst, src); } catch(Exception) { //обработка исключения }

Well, etc., but I'm not sure about the correct approach. I would be very grateful for the advice.

Implementing the path generation method for the file

 private string generatePathForAttachment(params string[] args) { var dstPath = string.Empty; var srcString = string.Empty; if(args.Length==0) { srcString = DateTime.Now.ToString(); } else { srcString = string.Concat(args.Select(x=>x)); } var hash = getHashString(srcString); dstPath = Path.Combine(hash.Substring(0, 2), hash.Substring(2, 2), hash.Substring(4)); return dstPath; } private string getHashString(string s) { string hash = string.Empty; //переводим строку в байт-массим byte[] bytes = Encoding.Unicode.GetBytes(s); //создаем объект для получения средст шифрования MD5CryptoServiceProvider CSP = new MD5CryptoServiceProvider(); //вычисляем хеш-представление в байтах byte[] byteHash = CSP.ComputeHash(bytes); //формируем одну цельную строку из массива foreach (byte b in byteHash) hash += string.Format("{0:x2}", b); return hash; }

In fact, the server is a virtual machine, only I (as a developer) and the administrator have access to the directory along the program (but he does not go there)

  • Comments are not intended for extended discussion; conversation moved to chat . - Nick Volynkin ♦

1 answer 1

What is required from the code? I understand that two requirements must be met:

  1. If there is an entry in the database about the file, then it must exist.
  2. If the operation failed, you must report the error to the person who called the code.

Both of these requirements are met. In this regard, the code is completely correct.

After fulfilling the basic requirements, you usually want to make the code fall even less often with an error. I have to say, your suggestions are stupid.

So, Directory.CreateDirectory will not do anything if the directory has already been created. File.Move also checks for the existence of the source file and the non-existence of the target files, an additional check here will only interfere. Do not check for standard functions that they should check themselves.

Instead, you should think about the race. A race occurs whenever you use File.Exist - because another program (or the second stream of yours) can create or delete the necessary file immediately after checking :)

You use this check in the while condition when looking for a free name for the file — but the final name free check happens only in the File.Move method. Hence the conclusion - it is necessary to stretch the loop so that it gets a call to File.Move . The problem here is to separate the move error due to the existence of the target file from the move error for other reasons:

 AttachedFile attachedFile; for(;;) { filePath = generatePathForAttachment(); dst = Path.Combine(archiveDirectory, filePath); Directory.CreateDirectory(Path.GetDirectoryName(dst)); attachedFile = new AttachedFile { OriginalTitle = attachment.OriginalTitle, Path = filePath, Size = attachment.Stream.Length, AuthorId = attachment.AuthorId, Created = DateTime.Now }; try { File.Move(tmpFile, dst); } catch { if (File.Exists(dst)) continue; else throw; } break; }

Here I use File.Exists check - but after calling File.Move . If other programs do not delete newly created files, then there will be no race.

PS what size files are supposed to be? If a small - it is easier to store them in the database.

  • file sizes vary greatly, from a few kilobytes to several hundred megabytes, so initially I decided not to store files in the database - Bald