View Issue Details

Jump to NotesJump to History
IDProjectCategoryView StatusDate SubmittedLast Update
0001157OpenMPTGeneralpublic2018-10-23 05:242018-11-09 07:34
Reporterfuzion_mixer Assigned Tomanx  
PriorityhighSeveritymajorReproducibilitysometimes
Status resolvedResolutionfixed 
Platformx64OSWindowsOS Version10
Product VersionOpenMPT 1.27.10.00 / libopenmpt 0.3.12 (upgrade first) 
Target VersionOpenMPT 1.28.01.00 / libopenmpt 0.4.0 (upgrade first)Fixed in VersionOpenMPT 1.28.01.00 / libopenmpt 0.4.0 (upgrade first) 
Summary0001157: Corrupted filesave
Description

Since my laptop has a battery problem where I can only turn it on via AC, I had no choice but to shut it down and start it up a couple times a day. However, doing this sometimes results in a corrupted save.
The corrupted savefile: (link to file full of zero bytes removed)

Steps To Reproduce
  1. Save the file
  2. Close OpenMPT immediately
  3. Restart computer
  4. Repeat if necessary
Additional Information

It might be due to the repeated shutting down / starting up that's messing with the file contents.

TagsNo tags attached.
Attached Files
file-flush-v2.patch (14,421 bytes)    
Index: common/mptFileIO.h
===================================================================
--- common/mptFileIO.h	(revision 10955)
+++ common/mptFileIO.h	(working copy)
@@ -23,6 +23,14 @@
 #include <streambuf>
 #include <utility>
 
+#if MPT_COMPILER_MSVC
+#include <cstdio>
+#endif // !MPT_COMPILER_MSVC
+
+#if MPT_COMPILER_MSVC
+#include <stdio.h>
+#endif // !MPT_COMPILER_MSVC
+
 #endif // MPT_ENABLE_FILEIO
 
 
@@ -125,6 +133,14 @@
 	{
 		detail::fstream_open<Tbase>(*this, filename, mode);
 	}
+#if MPT_COMPILER_MSVC
+protected:
+	ofstream(FILE * file)
+		: std::ofstream(file)
+	{
+	}
+#endif // MPT_COMPILER_MSVC
+public:
 	void open(const mpt::PathString & filename, std::ios_base::openmode mode = std::ios_base::out)
 	{
 		detail::fstream_open<Tbase>(*this, filename, mode);
@@ -137,8 +153,123 @@
 #endif
 };
 
+class SafeOutputFile
+	: public mpt::ofstream
+{
+private:
+	typedef std::ofstream Tbase;
+#if MPT_COMPILER_MSVC
+	FILE *m_f;
+#endif // MPT_COMPILER_MSVC
+#if MPT_COMPILER_MSVC
+	static mpt::tstring convert_mode(std::ios_base::openmode mode)
+	{
+		mpt::tstring fopen_mode;
+		switch(mode & ~(std::ios_base::ate | std::ios_base::binary))
+		{
+		case std::ios_base::in:
+			fopen_mode = TEXT("r");
+			break;
+		case std::ios_base::out:
+			MPT_FALLTHROUGH;
+		case std::ios_base::out | std::ios_base::trunc:
+			fopen_mode = TEXT("w");
+			break;
+		case std::ios_base::app:
+			MPT_FALLTHROUGH;
+		case std::ios_base::out | std::ios_base::app:
+			fopen_mode = TEXT("a");
+			break;
+		case std::ios_base::out | std::ios_base::in:
+			fopen_mode = TEXT("r+");
+			break;
+		case std::ios_base::out | std::ios_base::in | std::ios_base::trunc:
+			fopen_mode = TEXT("w+");
+			break;
+		case std::ios_base::out | std::ios_base::in | std::ios_base::app:
+			MPT_FALLTHROUGH;
+		case std::ios_base::in | std::ios_base::app:
+			fopen_mode = TEXT("a+");
+			break;
+		}
+		if(fopen_mode.empty())
+		{
+			return fopen_mode;
+		}
+		if(mode & std::ios_base::binary)
+		{
+			fopen_mode += TEXT("b");
+		}
+		fopen_mode += TEXT("c");  // force commit on fflush (MSVC specific)
+		return fopen_mode;
+	}
+	FILE * internal_fopen(const mpt::PathString &filename, std::ios_base::openmode mode)
+	{
+		mpt::tstring fopen_mode = convert_mode(mode);
+		if(fopen_mode.empty())
+		{
+			return nullptr;
+		}
+		FILE *f =
+#ifdef UNICODE
+			_wfopen(filename.AsNativePrefixed().c_str(), fopen_mode.c_str())
+#else
+			fopen(filename.AsNativePrefixed().c_str(), fopen_mode.c_str())
+#endif
+			;
+		if(!f)
+		{
+			return nullptr;
+		}
+		if(mode & std::ios_base::ate)
+		{
+			if(fseek(f, 0, SEEK_END) != 0)
+			{
+				fclose(f);
+				f = nullptr;
+				return nullptr;
+			}
+		}
+		m_f = f;
+		return f;
+	}
+#endif // MPT_COMPILER_MSVC
+public:
+	SafeOutputFile() = delete;
+	explicit SafeOutputFile(const mpt::PathString &filename, std::ios_base::openmode mode = std::ios_base::out)
+#if MPT_COMPILER_MSVC
+		: mpt::ofstream(internal_fopen(filename, mode | std::ios_base::out))
+#else // !MPT_COMPILER_MSVC
+		: mpt::ofstream(filename, mode)
+#endif // MPT_COMPILER_MSVC
+	{
+	}
+	~SafeOutputFile()
+	{
+		if(!*this)
+		{
+			return;
+		}
+		if(!rdbuf())
+		{
+			return;
+		}
+		#if MPT_COMPILER_MSVC
+			if(!m_f)
+			{
+				return;
+			}
+		#endif // MPT_COMPILER_MSVC
+		rdbuf()->pubsync();
+		#if MPT_COMPILER_MSVC
+			fflush(m_f);
+			fclose(m_f);
+		#endif // MPT_COMPILER_MSVC
+	}
+};
 
 
+
 // LazyFileRef is a simple reference to an on-disk file by the means of a
 // filename which allows easy assignment of the whole file contents to and from
 // byte buffers.
@@ -223,5 +354,6 @@
 #endif // MPT_ENABLE_FILEIO
 
 
+
 OPENMPT_NAMESPACE_END
 
Index: mptrack/CommandSet.cpp
===================================================================
--- mptrack/CommandSet.cpp	(revision 10955)
+++ mptrack/CommandSet.cpp	(working copy)
@@ -1510,7 +1510,7 @@
 ...
 */
 
-	mpt::ofstream f(filename);
+	mpt::SafeOutputFile f(filename);
 	if(!f)
 	{
 		ErrorBox(IDS_CANT_OPEN_FILE_FOR_WRITING);
Index: mptrack/ExceptionHandler.cpp
===================================================================
--- mptrack/ExceptionHandler.cpp	(revision 10955)
+++ mptrack/ExceptionHandler.cpp	(working copy)
@@ -268,13 +268,13 @@
 	errorMessage += MPT_ULITERAL("\n\n");
 
 	{
-		mpt::ofstream f(crashDirectory.path + MPT_PATHSTRING("error.txt"), std::ios::binary);
+		mpt::SafeOutputFile f(crashDirectory.path + MPT_PATHSTRING("error.txt"), std::ios::binary);
 		f.imbue(std::locale::classic());
 		f << mpt::String::Replace(mpt::ToCharset(mpt::CharsetUTF8, errorMessage), "\n", "\r\n");
 	}
 
 	{
-		mpt::ofstream f(crashDirectory.path + MPT_PATHSTRING("threads.txt"), std::ios::binary);
+		mpt::SafeOutputFile f(crashDirectory.path + MPT_PATHSTRING("threads.txt"), std::ios::binary);
 		f.imbue(std::locale::classic());
 		f << mpt::format("current : %1")(mpt::fmt::hex0<8>(GetCurrentThreadId())) << "\r\n";
 		f << mpt::format("GUI     : %1")(mpt::fmt::hex0<8>(mpt::log::Trace::GetThreadId(mpt::log::Trace::ThreadKindGUI))) << "\r\n";
@@ -290,7 +290,7 @@
 	};
 
 	{
-		mpt::ofstream f(crashDirectory.path + MPT_PATHSTRING("active-settings.txt"), std::ios::binary);
+		mpt::SafeOutputFile f(crashDirectory.path + MPT_PATHSTRING("active-settings.txt"), std::ios::binary);
 		f.imbue(std::locale::classic());
 		if(&theApp.GetSettings())
 		{
@@ -357,7 +357,7 @@
 	*/
 
 	{
-		mpt::ofstream f(crashDirectory.path + MPT_PATHSTRING("about-openmpt.txt"), std::ios::binary);
+		mpt::SafeOutputFile f(crashDirectory.path + MPT_PATHSTRING("about-openmpt.txt"), std::ios::binary);
 		f.imbue(std::locale::classic());
 		f << mpt::ToCharset(mpt::CharsetUTF8, CAboutDlg::GetTabText(0));
 	}
Index: mptrack/mod2midi.cpp
===================================================================
--- mptrack/mod2midi.cpp	(revision 10955)
+++ mptrack/mod2midi.cpp	(working copy)
@@ -695,7 +695,7 @@
 
 void CDoMidiConvert::Run()
 {
-	mpt::ofstream f(m_fileName, std::ios::binary);
+	mpt::SafeOutputFile f(m_fileName, std::ios::binary);
 	if(!f.good())
 	{
 		Reporting::Error("Could not open file for writing. Is it open in another application?");
Index: mptrack/Mod2wave.cpp
===================================================================
--- mptrack/Mod2wave.cpp	(revision 10955)
+++ mptrack/Mod2wave.cpp	(working copy)
@@ -949,7 +949,7 @@
 		normalizeFile.open(normalizeFileName, std::ios::binary | std::ios::in | std::ios::out | std::ios::trunc);
 	}
 
-	mpt::ofstream fileStream(m_lpszFileName, std::ios::binary);
+	mpt::SafeOutputFile fileStream(m_lpszFileName, std::ios::binary);
 
 	if(!fileStream)
 	{
Index: mptrack/Moddoc.cpp
===================================================================
--- mptrack/Moddoc.cpp	(revision 10955)
+++ mptrack/Moddoc.cpp	(working copy)
@@ -278,7 +278,7 @@
 		return FALSE;
 
 	BOOL ok = FALSE;
-	mpt::ofstream f(filename, std::ios::binary);
+	mpt::SafeOutputFile f(filename, std::ios::binary);
 	if(f)
 	{
 		BeginWaitCursor();
@@ -1898,7 +1898,7 @@
 	if(!dlg.Show()) return;
 
 	filename = dlg.GetFirstFile();
-	mpt::ofstream f(filename, std::ios::binary);
+	mpt::SafeOutputFile f(filename, std::ios::binary);
 	if(!f)
 		return;
 
Index: mptrack/Modedit.cpp
===================================================================
--- mptrack/Modedit.cpp	(revision 10955)
+++ mptrack/Modedit.cpp	(working copy)
@@ -1099,7 +1099,7 @@
 	CStringA s;
 	EnvelopeToString(s, pIns->GetEnvelope(nEnv));
 
-	mpt::ofstream f(fileName, std::ios::binary);
+	mpt::SafeOutputFile f(fileName, std::ios::binary);
 	if(!f)
 	{
 		EndWaitCursor();
Index: mptrack/Settings.cpp
===================================================================
--- mptrack/Settings.cpp	(revision 10955)
+++ mptrack/Settings.cpp	(working copy)
@@ -490,7 +490,7 @@
 static void WriteFileUTF16LE(const mpt::PathString &filename, const std::wstring &str)
 {
 	STATIC_ASSERT(sizeof(wchar_t) == 2);
-	mpt::ofstream inifile(filename, std::ios::binary);
+	mpt::SafeOutputFile inifile(filename, std::ios::binary);
 	const uint8 UTF16LE_BOM[] = { 0xff, 0xfe };
 	inifile.write(reinterpret_cast<const char*>(UTF16LE_BOM), 2);
 	inifile.write(reinterpret_cast<const char*>(str.c_str()), str.length() * sizeof(std::wstring::value_type));
Index: mptrack/TrackerSettings.cpp
===================================================================
--- mptrack/TrackerSettings.cpp	(revision 10955)
+++ mptrack/TrackerSettings.cpp	(working copy)
@@ -786,7 +786,7 @@
 		if(!fn.FileOrDirectoryExists())
 		{
 			CTuning * pT = CSoundFile::CreateTuning12TET("12TET");
-			mpt::ofstream f(fn, std::ios::binary);
+			mpt::SafeOutputFile f(fn, std::ios::binary);
 			pT->Serialize(f);
 			f.close();
 			delete pT;
@@ -798,7 +798,7 @@
 		if(!fn.FileOrDirectoryExists())
 		{
 			CTuning * pT = CSoundFile::CreateTuning12TET("12TET [[fs15 1.17.02.49]]");
-			mpt::ofstream f(fn, std::ios::binary);
+			mpt::SafeOutputFile f(fn, std::ios::binary);
 			pT->Serialize(f);
 			f.close();
 			delete pT;
Index: mptrack/TuningDialog.cpp
===================================================================
--- mptrack/TuningDialog.cpp	(revision 10955)
+++ mptrack/TuningDialog.cpp	(working copy)
@@ -617,7 +617,7 @@
 
 		BeginWaitCursor();
 
-		mpt::ofstream fout(dlg.GetFirstFile(), std::ios::binary);
+		mpt::SafeOutputFile fout(dlg.GetFirstFile(), std::ios::binary);
 
 		if(tuningFilter != -1 && filterIndex == tuningFilter)
 		{
@@ -691,7 +691,7 @@
 			SanitizeFilename(nameW);
 			fileNameW = mpt::String::Replace(fileNameW, MPT_USTRING("%tuning_name%"), nameW);
 			fileName = mpt::PathString::FromUnicode(fileNameW);
-			mpt::ofstream fout(fileName, std::ios::binary);
+			mpt::SafeOutputFile fout(fileName, std::ios::binary);
 			if(tuning.Serialize(fout) != Tuning::SerializationResult::Success)
 			{
 				failure = true;
Index: soundlib/Load_it.cpp
===================================================================
--- soundlib/Load_it.cpp	(revision 10955)
+++ soundlib/Load_it.cpp	(working copy)
@@ -1364,7 +1364,6 @@
 
 bool CSoundFile::SaveIT(std::ostream &f, const mpt::PathString &filename, bool compatibilityExport)
 {
-	if(!f) return false;
 
 	const CModSpecifications &specs = (GetType() == MOD_TYPE_MPT ? ModSpecs::mptm : (compatibilityExport ? ModSpecs::it : ModSpecs::itEx));
 
Index: soundlib/Load_mod.cpp
===================================================================
--- soundlib/Load_mod.cpp	(revision 10955)
+++ soundlib/Load_mod.cpp	(working copy)
@@ -2184,7 +2184,10 @@
 
 bool CSoundFile::SaveMod(std::ostream &f) const
 {
-	if(!f || m_nChannels == 0) return false;
+	if(m_nChannels == 0)
+	{
+		return false;
+	}
 
 	// Write song title
 	{
Index: soundlib/Load_s3m.cpp
===================================================================
--- soundlib/Load_s3m.cpp	(revision 10955)
+++ soundlib/Load_s3m.cpp	(working copy)
@@ -610,7 +610,10 @@
 		0x80, 0x80, 0x80, 0x80, 0x80, 0x80, 0x80, 0x80,
 	};
 
-	if(!f || m_nChannels == 0) return false;
+	if(m_nChannels == 0)
+	{
+		return false;
+	}
 
 	const bool saveMuteStatus =
 #ifdef MODPLUG_TRACKER
Index: soundlib/Load_xm.cpp
===================================================================
--- soundlib/Load_xm.cpp	(revision 10955)
+++ soundlib/Load_xm.cpp	(working copy)
@@ -749,7 +749,6 @@
 
 bool CSoundFile::SaveXM(std::ostream &f, bool compatibilityExport)
 {
-	if(!f) return false;
 
 	bool addChannel = false; // avoid odd channel count for FT2 compatibility
 
Index: soundlib/plugins/PlugInterface.cpp
===================================================================
--- soundlib/plugins/PlugInterface.cpp	(revision 10955)
+++ soundlib/plugins/PlugInterface.cpp	(working copy)
@@ -652,7 +652,7 @@
 
 	bool bank = (dlg.GetExtension() == MPT_PATHSTRING("fxb"));
 
-	mpt::ofstream f(dlg.GetFirstFile(), std::ios::binary);
+	mpt::SafeOutputFile f(dlg.GetFirstFile(), std::ios::binary);
 	if(f.good() && VSTPresets::SaveFile(f, *this, bank))
 	{
 		return true;
Index: soundlib/SampleFormatFLAC.cpp
===================================================================
--- soundlib/SampleFormatFLAC.cpp	(revision 10955)
+++ soundlib/SampleFormatFLAC.cpp	(working copy)
@@ -533,7 +533,7 @@
 {
 #ifdef MPT_WITH_FLAC
 
-	mpt::ofstream f(filename, std::ios::binary);
+	mpt::SafeOutputFile f(filename, std::ios::binary);
 	if(!f)
 	{
 		return false;
Index: soundlib/SampleFormats.cpp
===================================================================
--- soundlib/SampleFormats.cpp	(revision 10955)
+++ soundlib/SampleFormats.cpp	(working copy)
@@ -539,7 +539,7 @@
 #ifndef MODPLUG_NO_FILESAVE
 bool CSoundFile::SaveWAVSample(SAMPLEINDEX nSample, const mpt::PathString &filename) const
 {
-	mpt::ofstream f(filename, std::ios::binary);
+	mpt::SafeOutputFile f(filename, std::ios::binary);
 	if(!f)
 	{
 		return false;
@@ -829,7 +829,7 @@
 
 bool CSoundFile::SaveRAWSample(SAMPLEINDEX nSample, const mpt::PathString &filename) const
 {
-	mpt::ofstream f(filename, std::ios::binary);
+	mpt::SafeOutputFile f(filename, std::ios::binary);
 	if(!f)
 	{
 		return false;
@@ -1170,7 +1170,7 @@
 
 bool CSoundFile::SaveS3ISample(SAMPLEINDEX smp, const mpt::PathString &filename) const
 {
-	mpt::ofstream f(filename, std::ios::binary);
+	mpt::SafeOutputFile f(filename, std::ios::binary);
 	if(!f)
 		return false;
 
@@ -1342,7 +1342,7 @@
 		return false;
 	}
 
-	mpt::ofstream f(filename, std::ios::binary);
+	mpt::SafeOutputFile f(filename, std::ios::binary);
 	if(!f)
 	{
 		return false;
@@ -2160,7 +2160,7 @@
 	{
 		return false;
 	}
-	mpt::ofstream f(filename, std::ios::binary);
+	mpt::SafeOutputFile f(filename, std::ios::binary);
 	if(!f.good())
 	{
 		return false;
@@ -3255,7 +3255,7 @@
 	ModInstrument *pIns = Instruments[nInstr];
 
 	if((!pIns) || filename.empty()) return false;
-	mpt::ofstream f(filename, std::ios::binary);
+	mpt::SafeOutputFile f(filename, std::ios::binary);
 	if(!f)
 	{
 		return false;
Index: soundlib/tuningCollection.cpp
===================================================================
--- soundlib/tuningCollection.cpp	(revision 10955)
+++ soundlib/tuningCollection.cpp	(working copy)
@@ -281,7 +281,7 @@
 			error = true;
 		} else
 		{
-			mpt::ofstream fout(fn, std::ios::binary);
+			mpt::SafeOutputFile fout(fn, std::ios::binary);
 			if(tuning.Serialize(fout) != Tuning::SerializationResult::Success)
 			{
 				error = true;
file-flush-v2.patch (14,421 bytes)   
file-flush-v4.patch (19,391 bytes)    
Index: common/mptFileIO.cpp
===================================================================
--- common/mptFileIO.cpp	(revision 10963)
+++ common/mptFileIO.cpp	(working copy)
@@ -18,7 +18,13 @@
 #endif // MPT_OS_WINDOWS
 #endif // MODPLUG_TRACKER
 
+#if defined(MPT_ENABLE_FILEIO)
+#if MPT_COMPILER_MSVC
+#include <tchar.h>
+#endif // MPT_COMPILER_MSVC
+#endif // MPT_ENABLE_FILEIO
 
+
 OPENMPT_NAMESPACE_BEGIN
 
 
@@ -80,7 +86,62 @@
 
 
 namespace mpt {
+	
+#if MPT_COMPILER_MSVC
 
+mpt::tstring SafeOutputFile::convert_mode(std::ios_base::openmode mode, FlushMode flushMode)
+{
+	mpt::tstring fopen_mode;
+	switch(mode & ~(std::ios_base::ate | std::ios_base::binary))
+	{
+	case std::ios_base::in:
+		fopen_mode = _T("r");
+		break;
+	case std::ios_base::out:
+		MPT_FALLTHROUGH;
+	case std::ios_base::out | std::ios_base::trunc:
+		fopen_mode = _T("w");
+		break;
+	case std::ios_base::app:
+		MPT_FALLTHROUGH;
+	case std::ios_base::out | std::ios_base::app:
+		fopen_mode = _T("a");
+		break;
+	case std::ios_base::out | std::ios_base::in:
+		fopen_mode = _T("r+");
+		break;
+	case std::ios_base::out | std::ios_base::in | std::ios_base::trunc:
+		fopen_mode = _T("w+");
+		break;
+	case std::ios_base::out | std::ios_base::in | std::ios_base::app:
+		MPT_FALLTHROUGH;
+	case std::ios_base::in | std::ios_base::app:
+		fopen_mode = _T("a+");
+		break;
+	}
+	if(fopen_mode.empty())
+	{
+		return fopen_mode;
+	}
+	if(mode & std::ios_base::binary)
+	{
+		fopen_mode += _T("b");
+	}
+	if(flushMode == FlushMode::Full)
+	{
+		fopen_mode += _T("c");  // force commit on fflush (MSVC specific)
+	}
+	return fopen_mode;
+}
+
+#endif // MPT_COMPILER_MSVC
+
+} // namespace mpt
+
+
+
+namespace mpt {
+
 LazyFileRef & LazyFileRef::operator = (const std::vector<mpt::byte> &data)
 {
 	mpt::ofstream file(m_Filename, std::ios::binary);
Index: common/mptFileIO.h
===================================================================
--- common/mptFileIO.h	(revision 10963)
+++ common/mptFileIO.h	(working copy)
@@ -23,6 +23,14 @@
 #include <streambuf>
 #include <utility>
 
+#if MPT_COMPILER_MSVC
+#include <cstdio>
+#endif // !MPT_COMPILER_MSVC
+
+#if MPT_COMPILER_MSVC
+#include <stdio.h>
+#endif // !MPT_COMPILER_MSVC
+
 #endif // MPT_ENABLE_FILEIO
 
 
@@ -125,6 +133,14 @@
 	{
 		detail::fstream_open<Tbase>(*this, filename, mode);
 	}
+#if MPT_COMPILER_MSVC
+protected:
+	ofstream(FILE * file)
+		: std::ofstream(file)
+	{
+	}
+#endif // MPT_COMPILER_MSVC
+public:
 	void open(const mpt::PathString & filename, std::ios_base::openmode mode = std::ios_base::out)
 	{
 		detail::fstream_open<Tbase>(*this, filename, mode);
@@ -137,8 +153,103 @@
 #endif
 };
 
+enum class FlushMode
+{
+	None   = 0,  // no explicit flushes at all
+	Single = 1,  // explicitly flush higher-leverl API layers
+	Full   = 2,  // explicitly flush *all* layers, up to and including disk write caches
+};
 
+static inline FlushMode FlushModeFromBool(bool flush)
+{
+	return flush ? FlushMode::Full : FlushMode::None;
+}
 
+class SafeOutputFile
+	: public mpt::ofstream
+{
+private:
+	typedef std::ofstream Tbase;
+	FlushMode m_FlushMode;
+#if MPT_COMPILER_MSVC
+	FILE *m_f;
+#endif // MPT_COMPILER_MSVC
+#if MPT_COMPILER_MSVC
+	static mpt::tstring convert_mode(std::ios_base::openmode mode, FlushMode flushMode);
+	FILE * internal_fopen(const mpt::PathString &filename, std::ios_base::openmode mode, FlushMode flushMode)
+	{
+		mpt::tstring fopen_mode = convert_mode(mode, flushMode);
+		if(fopen_mode.empty())
+		{
+			return nullptr;
+		}
+		FILE *f =
+#ifdef UNICODE
+			_wfopen(filename.AsNativePrefixed().c_str(), fopen_mode.c_str())
+#else
+			fopen(filename.AsNativePrefixed().c_str(), fopen_mode.c_str())
+#endif
+			;
+		if(!f)
+		{
+			return nullptr;
+		}
+		if(mode & std::ios_base::ate)
+		{
+			if(fseek(f, 0, SEEK_END) != 0)
+			{
+				fclose(f);
+				f = nullptr;
+				return nullptr;
+			}
+		}
+		m_f = f;
+		return f;
+	}
+#endif // MPT_COMPILER_MSVC
+public:
+	SafeOutputFile() = delete;
+	explicit SafeOutputFile(const mpt::PathString &filename, std::ios_base::openmode mode = std::ios_base::out, FlushMode flushMode = FlushMode::Full)
+#if MPT_COMPILER_MSVC
+		: mpt::ofstream(internal_fopen(filename, mode | std::ios_base::out, flushMode))
+#else // !MPT_COMPILER_MSVC
+		: mpt::ofstream(filename, mode)
+#endif // MPT_COMPILER_MSVC
+		, m_FlushMode(flushMode)
+	{
+	}
+	~SafeOutputFile()
+	{
+		if(!*this)
+		{
+			return;
+		}
+		if(!rdbuf())
+		{
+			return;
+		}
+		#if MPT_COMPILER_MSVC
+			if(!m_f)
+			{
+				return;
+			}
+		#endif // MPT_COMPILER_MSVC
+		if(m_FlushMode != FlushMode::None)
+		{
+			rdbuf()->pubsync();
+		}
+		#if MPT_COMPILER_MSVC
+			if(m_FlushMode != FlushMode::None)
+			{
+				fflush(m_f);
+			}
+			fclose(m_f);
+		#endif // MPT_COMPILER_MSVC
+	}
+};
+
+
+
 // LazyFileRef is a simple reference to an on-disk file by the means of a
 // filename which allows easy assignment of the whole file contents to and from
 // byte buffers.
@@ -223,5 +334,6 @@
 #endif // MPT_ENABLE_FILEIO
 
 
+
 OPENMPT_NAMESPACE_END
 
Index: mptrack/CommandSet.cpp
===================================================================
--- mptrack/CommandSet.cpp	(revision 10963)
+++ mptrack/CommandSet.cpp	(working copy)
@@ -1510,7 +1510,7 @@
 ...
 */
 
-	mpt::ofstream f(filename);
+	mpt::SafeOutputFile f(filename, std::ios::out, mpt::FlushModeFromBool(TrackerSettings::Instance().MiscFlushFileBuffersOnSave));
 	if(!f)
 	{
 		ErrorBox(IDS_CANT_OPEN_FILE_FOR_WRITING);
Index: mptrack/ExceptionHandler.cpp
===================================================================
--- mptrack/ExceptionHandler.cpp	(revision 10963)
+++ mptrack/ExceptionHandler.cpp	(working copy)
@@ -268,13 +268,13 @@
 	errorMessage += MPT_ULITERAL("\n\n");
 
 	{
-		mpt::ofstream f(crashDirectory.path + MPT_PATHSTRING("error.txt"), std::ios::binary);
+		mpt::SafeOutputFile f(crashDirectory.path + MPT_PATHSTRING("error.txt"), std::ios::binary, mpt::FlushMode::Full);
 		f.imbue(std::locale::classic());
 		f << mpt::String::Replace(mpt::ToCharset(mpt::CharsetUTF8, errorMessage), "\n", "\r\n");
 	}
 
 	{
-		mpt::ofstream f(crashDirectory.path + MPT_PATHSTRING("threads.txt"), std::ios::binary);
+		mpt::SafeOutputFile f(crashDirectory.path + MPT_PATHSTRING("threads.txt"), std::ios::binary, mpt::FlushMode::Full);
 		f.imbue(std::locale::classic());
 		f << mpt::format("current : %1")(mpt::fmt::hex0<8>(GetCurrentThreadId())) << "\r\n";
 		f << mpt::format("GUI     : %1")(mpt::fmt::hex0<8>(mpt::log::Trace::GetThreadId(mpt::log::Trace::ThreadKindGUI))) << "\r\n";
@@ -290,7 +290,7 @@
 	};
 
 	{
-		mpt::ofstream f(crashDirectory.path + MPT_PATHSTRING("active-settings.txt"), std::ios::binary);
+		mpt::SafeOutputFile f(crashDirectory.path + MPT_PATHSTRING("active-settings.txt"), std::ios::binary, mpt::FlushMode::Full);
 		f.imbue(std::locale::classic());
 		if(&theApp.GetSettings())
 		{
@@ -357,13 +357,13 @@
 	*/
 
 	{
-		mpt::ofstream f(crashDirectory.path + MPT_PATHSTRING("about-openmpt.txt"), std::ios::binary);
+		mpt::SafeOutputFile f(crashDirectory.path + MPT_PATHSTRING("about-openmpt.txt"), std::ios::binary, mpt::FlushMode::Full);
 		f.imbue(std::locale::classic());
 		f << mpt::ToCharset(mpt::CharsetUTF8, CAboutDlg::GetTabText(0));
 	}
 
 	{
-		mpt::ofstream f(crashDirectory.path + MPT_PATHSTRING("about-components.txt"), std::ios::binary);
+		mpt::SafeOutputFile f(crashDirectory.path + MPT_PATHSTRING("about-components.txt"), std::ios::binary, mpt::FlushMode::Full);
 		f.imbue(std::locale::classic());
 		f << mpt::ToCharset(mpt::CharsetUTF8, CAboutDlg::GetTabText(1));
 	}
Index: mptrack/mod2midi.cpp
===================================================================
--- mptrack/mod2midi.cpp	(revision 10963)
+++ mptrack/mod2midi.cpp	(working copy)
@@ -695,7 +695,7 @@
 
 void CDoMidiConvert::Run()
 {
-	mpt::ofstream f(m_fileName, std::ios::binary);
+	mpt::SafeOutputFile f(m_fileName, std::ios::binary, mpt::FlushModeFromBool(TrackerSettings::Instance().MiscFlushFileBuffersOnSave));
 	if(!f.good())
 	{
 		Reporting::Error("Could not open file for writing. Is it open in another application?");
Index: mptrack/Mod2wave.cpp
===================================================================
--- mptrack/Mod2wave.cpp	(revision 10963)
+++ mptrack/Mod2wave.cpp	(working copy)
@@ -949,7 +949,7 @@
 		normalizeFile.open(normalizeFileName, std::ios::binary | std::ios::in | std::ios::out | std::ios::trunc);
 	}
 
-	mpt::ofstream fileStream(m_lpszFileName, std::ios::binary);
+	mpt::SafeOutputFile fileStream(m_lpszFileName, std::ios::binary, mpt::FlushModeFromBool(TrackerSettings::Instance().MiscFlushFileBuffersOnSave));
 
 	if(!fileStream)
 	{
Index: mptrack/Moddoc.cpp
===================================================================
--- mptrack/Moddoc.cpp	(revision 10963)
+++ mptrack/Moddoc.cpp	(working copy)
@@ -278,7 +278,7 @@
 		return FALSE;
 
 	BOOL ok = FALSE;
-	mpt::ofstream f(filename, std::ios::binary);
+	mpt::SafeOutputFile f(filename, std::ios::binary, mpt::FlushModeFromBool(TrackerSettings::Instance().MiscFlushFileBuffersOnSave));
 	if(f)
 	{
 		BeginWaitCursor();
@@ -1898,7 +1898,7 @@
 	if(!dlg.Show()) return;
 
 	filename = dlg.GetFirstFile();
-	mpt::ofstream f(filename, std::ios::binary);
+	mpt::SafeOutputFile f(filename, std::ios::binary, mpt::FlushModeFromBool(TrackerSettings::Instance().MiscFlushFileBuffersOnSave));
 	if(!f)
 		return;
 
Index: mptrack/Modedit.cpp
===================================================================
--- mptrack/Modedit.cpp	(revision 10963)
+++ mptrack/Modedit.cpp	(working copy)
@@ -1099,7 +1099,7 @@
 	CStringA s;
 	EnvelopeToString(s, pIns->GetEnvelope(nEnv));
 
-	mpt::ofstream f(fileName, std::ios::binary);
+	mpt::SafeOutputFile f(fileName, std::ios::binary, mpt::FlushModeFromBool(TrackerSettings::Instance().MiscFlushFileBuffersOnSave));
 	if(!f)
 	{
 		EndWaitCursor();
Index: mptrack/Settings.cpp
===================================================================
--- mptrack/Settings.cpp	(revision 10963)
+++ mptrack/Settings.cpp	(working copy)
@@ -490,7 +490,7 @@
 static void WriteFileUTF16LE(const mpt::PathString &filename, const std::wstring &str)
 {
 	STATIC_ASSERT(sizeof(wchar_t) == 2);
-	mpt::ofstream inifile(filename, std::ios::binary);
+	mpt::SafeOutputFile inifile(filename, std::ios::binary, mpt::FlushMode::Full);
 	const uint8 UTF16LE_BOM[] = { 0xff, 0xfe };
 	inifile.write(reinterpret_cast<const char*>(UTF16LE_BOM), 2);
 	inifile.write(reinterpret_cast<const char*>(str.c_str()), str.length() * sizeof(std::wstring::value_type));
Index: mptrack/TrackerSettings.cpp
===================================================================
--- mptrack/TrackerSettings.cpp	(revision 10963)
+++ mptrack/TrackerSettings.cpp	(working copy)
@@ -222,6 +222,7 @@
 	, MiscAllowMultipleCommandsPerKey(conf, MPT_USTRING("Misc"), MPT_USTRING("AllowMultipleCommandsPerKey"), false)
 	, MiscDistinguishModifiers(conf, MPT_USTRING("Misc"), MPT_USTRING("DistinguishModifiers"), false)
 	, MiscProcessPriorityClass(conf, MPT_USTRING("Misc"), MPT_USTRING("ProcessPriorityClass"), ProcessPriorityClassNORMAL)
+	, MiscFlushFileBuffersOnSave(conf, MPT_USTRING("Misc"), MPT_USTRING("FlushFileBuffersOnSave"), true)
 	// Sound Settings
 	, m_SoundShowDeprecatedDevices(conf, MPT_USTRING("Sound Settings"), MPT_USTRING("ShowDeprecatedDevices"), true)
 	, m_SoundShowNotRecommendedDeviceWarning(conf, MPT_USTRING("Sound Settings"), MPT_USTRING("ShowNotRecommendedDeviceWarning"), true)
@@ -786,7 +787,7 @@
 		if(!fn.FileOrDirectoryExists())
 		{
 			CTuning * pT = CSoundFile::CreateTuning12TET("12TET");
-			mpt::ofstream f(fn, std::ios::binary);
+			mpt::SafeOutputFile f(fn, std::ios::binary, mpt::FlushMode::Full);
 			pT->Serialize(f);
 			f.close();
 			delete pT;
@@ -798,7 +799,7 @@
 		if(!fn.FileOrDirectoryExists())
 		{
 			CTuning * pT = CSoundFile::CreateTuning12TET("12TET [[fs15 1.17.02.49]]");
-			mpt::ofstream f(fn, std::ios::binary);
+			mpt::SafeOutputFile f(fn, std::ios::binary, mpt::FlushMode::Full);
 			pT->Serialize(f);
 			f.close();
 			delete pT;
Index: mptrack/TrackerSettings.h
===================================================================
--- mptrack/TrackerSettings.h	(revision 10963)
+++ mptrack/TrackerSettings.h	(working copy)
@@ -637,6 +637,7 @@
 	CachedSetting<bool> MiscAllowMultipleCommandsPerKey;
 	CachedSetting<bool> MiscDistinguishModifiers;
 	Setting<ProcessPriorityClass> MiscProcessPriorityClass;
+	Setting<bool> MiscFlushFileBuffersOnSave;
 
 	// Sound Settings
 
Index: mptrack/TuningDialog.cpp
===================================================================
--- mptrack/TuningDialog.cpp	(revision 10963)
+++ mptrack/TuningDialog.cpp	(working copy)
@@ -617,7 +617,7 @@
 
 		BeginWaitCursor();
 
-		mpt::ofstream fout(dlg.GetFirstFile(), std::ios::binary);
+		mpt::SafeOutputFile fout(dlg.GetFirstFile(), std::ios::binary, mpt::FlushModeFromBool(TrackerSettings::Instance().MiscFlushFileBuffersOnSave));
 
 		if(tuningFilter != -1 && filterIndex == tuningFilter)
 		{
@@ -691,7 +691,7 @@
 			SanitizeFilename(nameW);
 			fileNameW = mpt::String::Replace(fileNameW, MPT_USTRING("%tuning_name%"), nameW);
 			fileName = mpt::PathString::FromUnicode(fileNameW);
-			mpt::ofstream fout(fileName, std::ios::binary);
+			mpt::SafeOutputFile fout(fileName, std::ios::binary, mpt::FlushModeFromBool(TrackerSettings::Instance().MiscFlushFileBuffersOnSave));
 			if(tuning.Serialize(fout) != Tuning::SerializationResult::Success)
 			{
 				failure = true;
Index: soundlib/Load_it.cpp
===================================================================
--- soundlib/Load_it.cpp	(revision 10963)
+++ soundlib/Load_it.cpp	(working copy)
@@ -1364,7 +1364,6 @@
 
 bool CSoundFile::SaveIT(std::ostream &f, const mpt::PathString &filename, bool compatibilityExport)
 {
-	if(!f) return false;
 
 	const CModSpecifications &specs = (GetType() == MOD_TYPE_MPT ? ModSpecs::mptm : (compatibilityExport ? ModSpecs::it : ModSpecs::itEx));
 
Index: soundlib/Load_mod.cpp
===================================================================
--- soundlib/Load_mod.cpp	(revision 10963)
+++ soundlib/Load_mod.cpp	(working copy)
@@ -2184,7 +2184,10 @@
 
 bool CSoundFile::SaveMod(std::ostream &f) const
 {
-	if(!f || m_nChannels == 0) return false;
+	if(m_nChannels == 0)
+	{
+		return false;
+	}
 
 	// Write song title
 	{
Index: soundlib/Load_s3m.cpp
===================================================================
--- soundlib/Load_s3m.cpp	(revision 10963)
+++ soundlib/Load_s3m.cpp	(working copy)
@@ -610,7 +610,10 @@
 		0x80, 0x80, 0x80, 0x80, 0x80, 0x80, 0x80, 0x80,
 	};
 
-	if(!f || m_nChannels == 0) return false;
+	if(m_nChannels == 0)
+	{
+		return false;
+	}
 
 	const bool saveMuteStatus =
 #ifdef MODPLUG_TRACKER
Index: soundlib/Load_xm.cpp
===================================================================
--- soundlib/Load_xm.cpp	(revision 10963)
+++ soundlib/Load_xm.cpp	(working copy)
@@ -749,7 +749,6 @@
 
 bool CSoundFile::SaveXM(std::ostream &f, bool compatibilityExport)
 {
-	if(!f) return false;
 
 	bool addChannel = false; // avoid odd channel count for FT2 compatibility
 
Index: soundlib/plugins/PlugInterface.cpp
===================================================================
--- soundlib/plugins/PlugInterface.cpp	(revision 10963)
+++ soundlib/plugins/PlugInterface.cpp	(working copy)
@@ -652,7 +652,7 @@
 
 	bool bank = (dlg.GetExtension() == MPT_PATHSTRING("fxb"));
 
-	mpt::ofstream f(dlg.GetFirstFile(), std::ios::binary);
+	mpt::SafeOutputFile f(dlg.GetFirstFile(), std::ios::binary, mpt::FlushModeFromBool(TrackerSettings::Instance().MiscFlushFileBuffersOnSave));
 	if(f.good() && VSTPresets::SaveFile(f, *this, bank))
 	{
 		return true;
Index: soundlib/SampleFormatFLAC.cpp
===================================================================
--- soundlib/SampleFormatFLAC.cpp	(revision 10963)
+++ soundlib/SampleFormatFLAC.cpp	(working copy)
@@ -533,7 +533,15 @@
 {
 #ifdef MPT_WITH_FLAC
 
-	mpt::ofstream f(filename, std::ios::binary);
+	const mpt::FlushMode flushMode = 
+		#ifdef MODPLUG_TRACKER
+			mpt::FlushModeFromBool(TrackerSettings::Instance().MiscFlushFileBuffersOnSave)
+		#else
+			mpt::FlushMode::Full
+		#endif
+		;
+
+	mpt::SafeOutputFile f(filename, std::ios::binary, flushMode);
 	if(!f)
 	{
 		return false;
Index: soundlib/SampleFormats.cpp
===================================================================
--- soundlib/SampleFormats.cpp	(revision 10963)
+++ soundlib/SampleFormats.cpp	(working copy)
@@ -39,6 +39,22 @@
 OPENMPT_NAMESPACE_BEGIN
 
 
+#ifndef MODPLUG_NO_FILESAVE
+
+static mpt::FlushMode GetSampleFileFlushMode()
+{
+	return
+		#ifdef MODPLUG_TRACKER
+			mpt::FlushModeFromBool(TrackerSettings::Instance().MiscFlushFileBuffersOnSave)
+		#else
+			mpt::FlushMode::Full
+		#endif
+		;
+}
+
+#endif // !MODPLUG_NO_FILESAVE
+
+
 bool CSoundFile::ReadSampleFromFile(SAMPLEINDEX nSample, FileReader &file, bool mayNormalize, bool includeInstrumentFormats)
 {
 	if(!nSample || nSample >= MAX_SAMPLES) return false;
@@ -539,7 +555,7 @@
 #ifndef MODPLUG_NO_FILESAVE
 bool CSoundFile::SaveWAVSample(SAMPLEINDEX nSample, const mpt::PathString &filename) const
 {
-	mpt::ofstream f(filename, std::ios::binary);
+	mpt::SafeOutputFile f(filename, std::ios::binary, GetSampleFileFlushMode());
 	if(!f)
 	{
 		return false;
@@ -829,7 +845,7 @@
 
 bool CSoundFile::SaveRAWSample(SAMPLEINDEX nSample, const mpt::PathString &filename) const
 {
-	mpt::ofstream f(filename, std::ios::binary);
+	mpt::SafeOutputFile f(filename, std::ios::binary, GetSampleFileFlushMode());
 	if(!f)
 	{
 		return false;
@@ -1170,7 +1186,7 @@
 
 bool CSoundFile::SaveS3ISample(SAMPLEINDEX smp, const mpt::PathString &filename) const
 {
-	mpt::ofstream f(filename, std::ios::binary);
+	mpt::SafeOutputFile f(filename, std::ios::binary, GetSampleFileFlushMode());
 	if(!f)
 		return false;
 
@@ -1342,7 +1358,7 @@
 		return false;
 	}
 
-	mpt::ofstream f(filename, std::ios::binary);
+	mpt::SafeOutputFile f(filename, std::ios::binary, GetSampleFileFlushMode());
 	if(!f)
 	{
 		return false;
@@ -2160,7 +2176,7 @@
 	{
 		return false;
 	}
-	mpt::ofstream f(filename, std::ios::binary);
+	mpt::SafeOutputFile f(filename, std::ios::binary, GetSampleFileFlushMode());
 	if(!f.good())
 	{
 		return false;
@@ -3255,7 +3271,7 @@
 	ModInstrument *pIns = Instruments[nInstr];
 
 	if((!pIns) || filename.empty()) return false;
-	mpt::ofstream f(filename, std::ios::binary);
+	mpt::SafeOutputFile f(filename, std::ios::binary, GetSampleFileFlushMode());
 	if(!f)
 	{
 		return false;
Index: soundlib/tuningCollection.cpp
===================================================================
--- soundlib/tuningCollection.cpp	(revision 10963)
+++ soundlib/tuningCollection.cpp	(working copy)
@@ -15,6 +15,9 @@
 #include <algorithm>
 #include "../common/mptFileIO.h"
 #include "Loaders.h"
+#ifdef MODPLUG_TRACKER
+#include "../mptrack/TrackerSettings.h"
+#endif //MODPLUG_TRACKER
 
 
 OPENMPT_NAMESPACE_BEGIN
@@ -281,7 +284,7 @@
 			error = true;
 		} else
 		{
-			mpt::ofstream fout(fn, std::ios::binary);
+			mpt::SafeOutputFile fout(fn, std::ios::binary, mpt::FlushModeFromBool(TrackerSettings::Instance().MiscFlushFileBuffersOnSave));
 			if(tuning.Serialize(fout) != Tuning::SerializationResult::Success)
 			{
 				error = true;
file-flush-v4.patch (19,391 bytes)   
Has the bug occurred in previous versions?The only other version I was using whilst working on the module is v1.27.09.00.
Tested code revision (in case you know it)

Relationships

related to 0001161 resolvedSaga Musix Notify user about file write failures 

Activities

manx

manx

2018-10-23 06:33

administrator   ~0003684

Does this happen when you are properly shutting down Windows (i.e. using shutdown or reboot from the start menu)? Or are you talking about a hard power loss without shutting down Windows properly (i.e. pressing the power button 5 seconds, or unplugging the power cable with no battery (or immediately failing battery), or is the battery too weak to properly finish shutting down?)?

Do you save the file to the internal C: drive or to some secondary internal drive or external drive like a USB key, SD card, external HDD/SSD, or network drive?

The file you provided is completely empty (i.e. it contains absolutely no recoverable data). This obviously should never happen, however I cannot imagine how that could happen if you are properly closing OpenMPT and then properly shutting down Windows. Hence the questions about the precise situation.

There is something that OpenMPT could do here: flushing the file buffers after saving. However that would (in theory) only make a difference if an actual Windows crash (as opposed to proper shutdown) happens right after saving.
fuzion_mixer

fuzion_mixer

2018-10-23 06:46

reporter   ~0003685

Hard power loss i.e removing AC without shutting the laptop down.

As for where I saved it, I saved it in a desktop directory where i keep all my module stuff [Desktop\MOD Stuff\Ideas (Modules)].
manx

manx

2018-10-23 09:55

administrator   ~0003686

@Saga Musix @{U4} :

This change implements file buffer flushing. This is a good idea in any case, not in particular because of this very issue, but because obscure audio hardware has at least a somewhat increased tendency to cause BSODs which will hit the exact same problem. Disadvantage of the change is being slightly slower file saving ((filesize / device_write_speed) + device_sync_time(about 30ms on mechanical media)).

@fuzion_mixer :

You should really try to avoid hard power losses if at all possible. This can corrupt all kinds of other software's state as well.

file-flush-v1.patch (14,929 bytes)    
Index: common/mptFileIO.h
===================================================================
--- common/mptFileIO.h	(revision 10938)
+++ common/mptFileIO.h	(working copy)
@@ -125,6 +125,14 @@
 	{
 		detail::fstream_open<Tbase>(*this, filename, mode);
 	}
+#if MPT_COMPILER_MSVC
+protected:
+	ofstream(FILE * file)
+		: std::ofstream(file)
+	{
+	}
+#endif // MPT_COMPILER_MSVC
+public:
 	void open(const mpt::PathString & filename, std::ios_base::openmode mode = std::ios_base::out)
 	{
 		detail::fstream_open<Tbase>(*this, filename, mode);
@@ -137,8 +145,123 @@
 #endif
 };
 
+class SafeOutputFile
+	: public mpt::ofstream
+{
+private:
+	typedef std::ofstream Tbase;
+#if MPT_COMPILER_MSVC
+	FILE *m_f;
+#endif // MPT_COMPILER_MSVC
+#if MPT_COMPILER_MSVC
+	static mpt::tstring convert_mode(std::ios_base::openmode mode)
+	{
+		mpt::tstring fopen_mode;
+		switch(mode & ~(std::ios_base::ate | std::ios_base::binary))
+		{
+		case std::ios_base::in:
+			fopen_mode = TEXT("r");
+			break;
+		case std::ios_base::out:
+			MPT_FALLTHROUGH;
+		case std::ios_base::out | std::ios_base::trunc:
+			fopen_mode = TEXT("w");
+			break;
+		case std::ios_base::app:
+			MPT_FALLTHROUGH;
+		case std::ios_base::out | std::ios_base::app:
+			fopen_mode = TEXT("a");
+			break;
+		case std::ios_base::out | std::ios_base::in:
+			fopen_mode = TEXT("r+");
+			break;
+		case std::ios_base::out | std::ios_base::in | std::ios_base::trunc:
+			fopen_mode = TEXT("w+");
+			break;
+		case std::ios_base::out | std::ios_base::in | std::ios_base::app:
+			MPT_FALLTHROUGH;
+		case std::ios_base::in | std::ios_base::app:
+			fopen_mode = TEXT("a+");
+			break;
+		}
+		if(fopen_mode.empty())
+		{
+			return fopen_mode;
+		}
+		if(mode & std::ios_base::binary)
+		{
+			fopen_mode += TEXT("b");
+		}
+		fopen_mode += TEXT("c");  // force commit on fflush (MSVC specific)
+		return fopen_mode;
+	}
+	FILE * internal_fopen(const mpt::PathString &filename, std::ios_base::openmode mode)
+	{
+		mpt::tstring fopen_mode = convert_mode(mode);
+		if(fopen_mode.empty())
+		{
+			return nullptr;
+		}
+		FILE *f =
+#ifdef UNICODE
+			_wfopen(filename.AsNativePrefixed().c_str(), fopen_mode.c_str())
+#else
+			fopen(filename.AsNativePrefixed().c_str(), fopen_mode.c_str())
+#endif
+			;
+		if(!f)
+		{
+			return nullptr;
+		}
+		if(mode & std::ios_base::ate)
+		{
+			if(fseek(f, 0, SEEK_END) != 0)
+			{
+				fclose(f);
+				f = nullptr;
+				return nullptr;
+			}
+		}
+		m_f = f;
+		return f;
+	}
+#endif // MPT_COMPILER_MSVC
+public:
+	SafeOutputFile() = delete;
+	explicit SafeOutputFile(const mpt::PathString &filename, std::ios_base::openmode mode = std::ios_base::out)
+#if MPT_COMPILER_MSVC
+		: mpt::ofstream(internal_fopen(filename, mode | std::ios_base::out))
+#else // !MPT_COMPILER_MSVC
+		: mpt::ofstream(filename, mode)
+#endif // MPT_COMPILER_MSVC
+	{
+	}
+	~SafeOutputFile()
+	{
+		if(!*this)
+		{
+			return;
+		}
+		if(!rdbuf())
+		{
+			return;
+		}
+		#if MPT_COMPILER_MSVC
+			if(!m_f)
+			{
+				return;
+			}
+		#endif // MPT_COMPILER_MSVC
+		rdbuf()->pubsync();
+		#if MPT_COMPILER_MSVC
+			fflush(m_f);
+			fclose(m_f);
+		#endif // MPT_COMPILER_MSVC
+	}
+};
 
 
+
 // LazyFileRef is a simple reference to an on-disk file by the means of a
 // filename which allows easy assignment of the whole file contents to and from
 // byte buffers.
@@ -223,5 +346,16 @@
 #endif // MPT_ENABLE_FILEIO
 
 
+
+#ifdef MODPLUG_TRACKER
+inline bool SyncFileData(mpt::ofstream & f)
+{
+	f.rdbuf()->pubsync();
+	return true;
+}
+#endif // MODPLUG_TRACKER
+
+
+
 OPENMPT_NAMESPACE_END
 
Index: mptrack/CommandSet.cpp
===================================================================
--- mptrack/CommandSet.cpp	(revision 10938)
+++ mptrack/CommandSet.cpp	(working copy)
@@ -1510,7 +1510,7 @@
 ...
 */
 
-	mpt::ofstream f(filename);
+	mpt::SafeOutputFile f(filename);
 	if(!f)
 	{
 		ErrorBox(IDS_CANT_OPEN_FILE_FOR_WRITING);
Index: mptrack/ExceptionHandler.cpp
===================================================================
--- mptrack/ExceptionHandler.cpp	(revision 10938)
+++ mptrack/ExceptionHandler.cpp	(working copy)
@@ -268,13 +268,13 @@
 	errorMessage += MPT_ULITERAL("\n\n");
 
 	{
-		mpt::ofstream f(crashDirectory.path + MPT_PATHSTRING("error.txt"), std::ios::binary);
+		mpt::SafeOutputFile f(crashDirectory.path + MPT_PATHSTRING("error.txt"), std::ios::binary);
 		f.imbue(std::locale::classic());
 		f << mpt::String::Replace(mpt::ToCharset(mpt::CharsetUTF8, errorMessage), "\n", "\r\n");
 	}
 
 	{
-		mpt::ofstream f(crashDirectory.path + MPT_PATHSTRING("threads.txt"), std::ios::binary);
+		mpt::SafeOutputFile f(crashDirectory.path + MPT_PATHSTRING("threads.txt"), std::ios::binary);
 		f.imbue(std::locale::classic());
 		f << mpt::format("current : %1")(mpt::fmt::hex0<8>(GetCurrentThreadId())) << "\r\n";
 		f << mpt::format("GUI     : %1")(mpt::fmt::hex0<8>(mpt::log::Trace::GetThreadId(mpt::log::Trace::ThreadKindGUI))) << "\r\n";
@@ -290,7 +290,7 @@
 	};
 
 	{
-		mpt::ofstream f(crashDirectory.path + MPT_PATHSTRING("active-settings.txt"), std::ios::binary);
+		mpt::SafeOutputFile f(crashDirectory.path + MPT_PATHSTRING("active-settings.txt"), std::ios::binary);
 		f.imbue(std::locale::classic());
 		if(&theApp.GetSettings())
 		{
@@ -357,7 +357,7 @@
 	*/
 
 	{
-		mpt::ofstream f(crashDirectory.path + MPT_PATHSTRING("about-openmpt.txt"), std::ios::binary);
+		mpt::SafeOutputFile f(crashDirectory.path + MPT_PATHSTRING("about-openmpt.txt"), std::ios::binary);
 		f.imbue(std::locale::classic());
 		f << mpt::ToCharset(mpt::CharsetUTF8, CAboutDlg::GetTabText(0));
 	}
Index: mptrack/mod2midi.cpp
===================================================================
--- mptrack/mod2midi.cpp	(revision 10938)
+++ mptrack/mod2midi.cpp	(working copy)
@@ -710,7 +710,7 @@
 
 void CDoMidiConvert::Run()
 {
-	mpt::ofstream f(m_fileName, std::ios::binary);
+	mpt::SafeOutputFile f(m_fileName, std::ios::binary);
 	if(!f.good())
 	{
 		Reporting::Error("Could not open file for writing. Is it open in another application?");
Index: mptrack/Mod2wave.cpp
===================================================================
--- mptrack/Mod2wave.cpp	(revision 10938)
+++ mptrack/Mod2wave.cpp	(working copy)
@@ -949,7 +949,7 @@
 		normalizeFile.open(normalizeFileName, std::ios::binary | std::ios::in | std::ios::out | std::ios::trunc);
 	}
 
-	mpt::ofstream fileStream(m_lpszFileName, std::ios::binary);
+	mpt::SafeOutputFile fileStream(m_lpszFileName, std::ios::binary);
 
 	if(!fileStream)
 	{
Index: mptrack/Modedit.cpp
===================================================================
--- mptrack/Modedit.cpp	(revision 10938)
+++ mptrack/Modedit.cpp	(working copy)
@@ -1099,7 +1099,7 @@
 	CStringA s;
 	EnvelopeToString(s, pIns->GetEnvelope(nEnv));
 
-	mpt::ofstream f(fileName, std::ios::binary);
+	mpt::SafeOutputFile f(fileName, std::ios::binary);
 	if(!f)
 	{
 		EndWaitCursor();
Index: mptrack/Settings.cpp
===================================================================
--- mptrack/Settings.cpp	(revision 10938)
+++ mptrack/Settings.cpp	(working copy)
@@ -490,7 +490,7 @@
 static void WriteFileUTF16LE(const mpt::PathString &filename, const std::wstring &str)
 {
 	STATIC_ASSERT(sizeof(wchar_t) == 2);
-	mpt::ofstream inifile(filename, std::ios::binary);
+	mpt::SafeOutputFile inifile(filename, std::ios::binary);
 	const uint8 UTF16LE_BOM[] = { 0xff, 0xfe };
 	inifile.write(reinterpret_cast<const char*>(UTF16LE_BOM), 2);
 	inifile.write(reinterpret_cast<const char*>(str.c_str()), str.length() * sizeof(std::wstring::value_type));
Index: mptrack/TrackerSettings.cpp
===================================================================
--- mptrack/TrackerSettings.cpp	(revision 10938)
+++ mptrack/TrackerSettings.cpp	(working copy)
@@ -786,7 +786,7 @@
 		if(!fn.FileOrDirectoryExists())
 		{
 			CTuning * pT = CSoundFile::CreateTuning12TET("12TET");
-			mpt::ofstream f(fn, std::ios::binary);
+			mpt::SafeOutputFile f(fn, std::ios::binary);
 			pT->Serialize(f);
 			f.close();
 			delete pT;
@@ -798,7 +798,7 @@
 		if(!fn.FileOrDirectoryExists())
 		{
 			CTuning * pT = CSoundFile::CreateTuning12TET("12TET [[fs15 1.17.02.49]]");
-			mpt::ofstream f(fn, std::ios::binary);
+			mpt::SafeOutputFile f(fn, std::ios::binary);
 			pT->Serialize(f);
 			f.close();
 			delete pT;
Index: mptrack/TuningDialog.cpp
===================================================================
--- mptrack/TuningDialog.cpp	(revision 10938)
+++ mptrack/TuningDialog.cpp	(working copy)
@@ -617,7 +617,7 @@
 
 		BeginWaitCursor();
 
-		mpt::ofstream fout(dlg.GetFirstFile(), std::ios::binary);
+		mpt::SafeOutputFile fout(dlg.GetFirstFile(), std::ios::binary);
 
 		if(tuningFilter != -1 && filterIndex == tuningFilter)
 		{
@@ -691,7 +691,7 @@
 			SanitizeFilename(nameW);
 			fileNameW = mpt::String::Replace(fileNameW, MPT_USTRING("%tuning_name%"), nameW);
 			fileName = mpt::PathString::FromUnicode(fileNameW);
-			mpt::ofstream fout(fileName, std::ios::binary);
+			mpt::SafeOutputFile fout(fileName, std::ios::binary);
 			if(tuning.Serialize(fout) != Tuning::SerializationResult::Success)
 			{
 				failure = true;
Index: soundlib/Load_it.cpp
===================================================================
--- soundlib/Load_it.cpp	(revision 10938)
+++ soundlib/Load_it.cpp	(working copy)
@@ -1375,7 +1375,7 @@
 	{
 		return false;
 	}
-	mpt::ofstream f(filename, std::ios::binary);
+	mpt::SafeOutputFile f(filename, std::ios::binary);
 	if(!f)
 	{
 		return false;
Index: soundlib/Load_mod.cpp
===================================================================
--- soundlib/Load_mod.cpp	(revision 10938)
+++ soundlib/Load_mod.cpp	(working copy)
@@ -2186,7 +2186,7 @@
 {
 
 	if(m_nChannels == 0 || filename.empty()) return false;
-	mpt::ofstream f(filename, std::ios::binary);
+	mpt::SafeOutputFile f(filename, std::ios::binary);
 	if(!f)
 	{
 		return false;
Index: soundlib/Load_s3m.cpp
===================================================================
--- soundlib/Load_s3m.cpp	(revision 10938)
+++ soundlib/Load_s3m.cpp	(working copy)
@@ -611,7 +611,7 @@
 	};
 
 	if(m_nChannels == 0 || filename.empty()) return false;
-	mpt::ofstream f(filename, std::ios::binary);
+	mpt::SafeOutputFile f(filename, std::ios::binary);
 	if(!f)
 	{
 		return false;
Index: soundlib/Load_xm.cpp
===================================================================
--- soundlib/Load_xm.cpp	(revision 10938)
+++ soundlib/Load_xm.cpp	(working copy)
@@ -753,7 +753,7 @@
 	{
 		return false;
 	}
-	mpt::ofstream f(filename, std::ios::binary);
+	mpt::SafeOutputFile f(filename, std::ios::binary);
 	if(!f)
 	{
 		return false;
Index: soundlib/plugins/PlugInterface.cpp
===================================================================
--- soundlib/plugins/PlugInterface.cpp	(revision 10938)
+++ soundlib/plugins/PlugInterface.cpp	(working copy)
@@ -669,7 +669,7 @@
 
 	bool bank = (dlg.GetExtension() == MPT_PATHSTRING("fxb"));
 
-	mpt::ofstream f(dlg.GetFirstFile(), std::ios::binary);
+	mpt::SafeOutputFile f(dlg.GetFirstFile(), std::ios::binary);
 	if(f.good() && VSTPresets::SaveFile(f, *this, bank))
 	{
 		return true;
Index: soundlib/SampleFormatFLAC.cpp
===================================================================
--- soundlib/SampleFormatFLAC.cpp	(revision 10938)
+++ soundlib/SampleFormatFLAC.cpp	(working copy)
@@ -470,11 +470,11 @@
 struct FLAC__StreamEncoder_RAII
 {
 	FLAC__StreamEncoder *encoder;
-	mpt::ofstream f;
+	mpt::ofstream & f;
 
 	operator FLAC__StreamEncoder *() { return encoder; }
 
-	FLAC__StreamEncoder_RAII() : encoder(FLAC__stream_encoder_new()) { }
+	FLAC__StreamEncoder_RAII(mpt::ofstream & f) : encoder(FLAC__stream_encoder_new()), f(f) { }
 	~FLAC__StreamEncoder_RAII()
 	{
 		FLAC__stream_encoder_delete(encoder);
@@ -532,7 +532,12 @@
 bool CSoundFile::SaveFLACSample(SAMPLEINDEX nSample, const mpt::PathString &filename) const
 {
 #ifdef MPT_WITH_FLAC
-	FLAC__StreamEncoder_RAII encoder;
+	mpt::SafeOutputFile f(filename, std::ios::binary);
+	if(!f)
+	{
+		return false;
+	}
+	FLAC__StreamEncoder_RAII encoder(f);
 	if(encoder == nullptr)
 	{
 		return false;
@@ -668,8 +673,7 @@
 	FLAC__int32 *sampleData = nullptr;
 	SmpLength numSamples = 0;
 
-	encoder.f.open(filename, std::ios::binary);
-	if(!encoder.f || FLAC__stream_encoder_init_stream(encoder, &FLAC__StreamEncoder_RAII::StreamEncoderWriteCallback, &FLAC__StreamEncoder_RAII::StreamEncoderSeekCallback, &FLAC__StreamEncoder_RAII::StreamEncoderTellCallback, nullptr, &encoder.f) != FLAC__STREAM_ENCODER_INIT_STATUS_OK)
+	if(FLAC__stream_encoder_init_stream(encoder, &FLAC__StreamEncoder_RAII::StreamEncoderWriteCallback, &FLAC__StreamEncoder_RAII::StreamEncoderSeekCallback, &FLAC__StreamEncoder_RAII::StreamEncoderTellCallback, nullptr, &encoder.f) != FLAC__STREAM_ENCODER_INIT_STATUS_OK)
 	{
 		goto fail;
 	}
Index: soundlib/SampleFormats.cpp
===================================================================
--- soundlib/SampleFormats.cpp	(revision 10938)
+++ soundlib/SampleFormats.cpp	(working copy)
@@ -539,7 +539,7 @@
 #ifndef MODPLUG_NO_FILESAVE
 bool CSoundFile::SaveWAVSample(SAMPLEINDEX nSample, const mpt::PathString &filename) const
 {
-	mpt::ofstream f(filename, std::ios::binary);
+	mpt::SafeOutputFile f(filename, std::ios::binary);
 	if(!f)
 	{
 		return false;
@@ -829,7 +829,7 @@
 
 bool CSoundFile::SaveRAWSample(SAMPLEINDEX nSample, const mpt::PathString &filename) const
 {
-	mpt::ofstream f(filename, std::ios::binary);
+	mpt::SafeOutputFile f(filename, std::ios::binary);
 	if(!f)
 	{
 		return false;
@@ -1170,7 +1170,7 @@
 
 bool CSoundFile::SaveS3ISample(SAMPLEINDEX smp, const mpt::PathString &filename) const
 {
-	mpt::ofstream f(filename, std::ios::binary);
+	mpt::SafeOutputFile f(filename, std::ios::binary);
 	if(!f)
 		return false;
 
@@ -1342,7 +1342,7 @@
 		return false;
 	}
 
-	mpt::ofstream f(filename, std::ios::binary);
+	mpt::SafeOutputFile f(filename, std::ios::binary);
 	if(!f)
 	{
 		return false;
@@ -2160,7 +2160,7 @@
 	{
 		return false;
 	}
-	mpt::ofstream f(filename, std::ios::binary);
+	mpt::SafeOutputFile f(filename, std::ios::binary);
 	if(!f.good())
 	{
 		return false;
@@ -3255,7 +3255,7 @@
 	ModInstrument *pIns = Instruments[nInstr];
 
 	if((!pIns) || filename.empty()) return false;
-	mpt::ofstream f(filename, std::ios::binary);
+	mpt::SafeOutputFile f(filename, std::ios::binary);
 	if(!f)
 	{
 		return false;
Index: soundlib/tuningCollection.cpp
===================================================================
--- soundlib/tuningCollection.cpp	(revision 10938)
+++ soundlib/tuningCollection.cpp	(working copy)
@@ -281,7 +281,7 @@
 			error = true;
 		} else
 		{
-			mpt::ofstream fout(fn, std::ios::binary);
+			mpt::SafeOutputFile fout(fn, std::ios::binary);
 			if(tuning.Serialize(fout) != Tuning::SerializationResult::Success)
 			{
 				error = true;
file-flush-v1.patch (14,929 bytes)   
Saga Musix

Saga Musix

2018-10-23 11:32

administrator   ~0003687

I'm not a huge fan of changin the operating system's implicit storage strategy, in particular since saving happens in the UI thread and can already block for a measurable amount of time if there are large (compressed) samples - which is annoying in particular when auto-saving - but if we can determine that it won't delay the process much more, I guess it's safe to do.

@fuzion_mixer: There really is no need to make this issue private. Private issues are meant for reporting issues that are relevant to security.
manx

manx

2018-10-23 12:07

administrator   ~0003688

Flushing file buffers when saving user data is considered common sense best practice: When saving is signaled as done by the UI, the user expects the saved state to survive any crash that happens later. Performance considerations, especially for autosave of course also apply. I guess we could also add an option so that users who know their system is unconditionally stable can disable the flushing for improved performance.

We could even go a step further by writing to a temp file first, flushing it, and then renaming the temp file onto the to-be-overwritten file. This is not as important on Windows as it is on Linux for crash resilience because directory contents is implicitly flushed on Windows. However it is still useful to guard against crashes (Windows or OpenMPT) and i/o errors during overwriting data (in which case the old data will be lost and the new data will not be finished yet).

I still have a Windows 7 box with mechanical storage. Precise measurements will be difficult, but I can probably provide some real-world ballpark numbers.
Saga Musix

Saga Musix

2018-10-23 12:10

administrator   ~0003689

Generally I think it might make sense to move file creation outside of the module savers (i.e. pass a file handle rather than a filename) - this makes error handling when file creation fails more consistent, and it would give us the possibility to handle autosave and regular save differently without having to pass even more parameters to each export function. I suggest we do that as part of this improvement.
manx

manx

2018-10-23 13:00

administrator   ~0003690

I agree, that will simplify things.
Saga Musix

Saga Musix

2018-10-28 20:50

administrator   ~0003693

Module savers now take a stream handle rather than a filename.
manx

manx

2018-11-04 11:25

administrator   ~0003696

This updated patch adds configurable file buffer flushing. Hidden option is [Misc]FlushFileBuffersOnSave (default true, i.e. the safe, and potentially slow default). A few cases like migrating ini file to unicode (only happens once) or crash dump files (better safe than sorry and performance is irrelevant in this case) do unconditionally always do full flushes.
Saga Musix

Saga Musix

2018-11-06 18:46

administrator   ~0003698

Another somewhat related thing we might want to consider is catching I/O errors when saving (in particular write failures in case the disk is full). Since it would make the export code unnecessarily complex when checking every write call there, we could enable iostream exceptions for badbit/failbit and catch them (both outside of the loader of course).
manx

manx

2018-11-06 18:58

administrator   ~0003699

Agreed, ultimately, we should have that goal in mind.

I do not think we can achieve that for 1.28 though. There are still code paths that are not 100% exception-safe. This also probably requires another round of consolidation of all the different file i/o code paths that we have.

For 1.28, I think just the buffer flushing as in the provided patch, is probably enough. Otherwise this would delay 1.28 even more.
Saga Musix

Saga Musix

2018-11-06 18:58

administrator   ~0003700

I will create a separate issue for it then.
Saga Musix

Saga Musix

2018-11-07 21:56

administrator   ~0003703

I have tested the code both with an internal SSD and an external (USB 2) HDD. The extra time can be nicely observed by the time between the busy mouse cursor turing back into a regular mouse cursor and the save button in the main toolbar getting depressed (this is due to EndWaitCursor being called before the file stream is destructed).
For testing I used a 20MB module. The difference on the SSD drive was a fraction of a second - noticeable but negligible - while on the HDD it was about a second.
An 80MB module took still took less than a second on the SSD and about 4 seconds extra on the HDD. So in this particular setup, flushing file output stalls the UI for an additional second per 20MB on my HDD (quite a realistic expectation).
StarWolf3000

StarWolf3000

2018-11-08 07:05

reporter   ~0003704

Do you have enabled caching for the external HDD? Windows does handle I/O operations differently, if either caching is enabled or fast-remove is enabled.
manx

manx

2018-11-08 13:29

administrator   ~0003705

I did not do any precise measurements yet, however I tested similarly sized modules on a 1TB 7200rpm SATA drive, and the delays are (of course) still noticeable, but about 4 times shorter than what you are seeing on an USB2 HDD. Which totally makes sense, as this is effectively limited by USB2 transfer speeds at that point.

I think we should commit the patch as is. The improved safety for user data warrants the slight performance regression. People who are annoyed by the regression in file saving performance can simply disable the flushing via the hidden option. This probably needs a release-notes entry though.

@StarWolf3000 The whole point of the change is to explicitly flush caches, such that data is permanently stored on disk and is crash resilient. I.e. we explicitly want the non-write-caching behaviour in all cases (even if caching is enabled).
Saga Musix

Saga Musix

2018-11-08 18:15

administrator   ~0003706

Yes, let's do it like that. I think the patch can be merged as-is.
manx

manx

2018-11-09 07:34

administrator   ~0003707

Implemented in r10965.

Issue History

Date Modified Username Field Change
2018-10-23 05:24 fuzion_mixer New Issue
2018-10-23 06:33 manx Note Added: 0003684
2018-10-23 06:37 manx Assigned To => manx
2018-10-23 06:37 manx Priority normal => high
2018-10-23 06:37 manx Severity minor => major
2018-10-23 06:37 manx Status new => assigned
2018-10-23 06:46 fuzion_mixer Note Added: 0003685
2018-10-23 09:55 manx File Added: file-flush-v1.patch
2018-10-23 09:55 manx Note Added: 0003686
2018-10-23 10:08 manx Status assigned => acknowledged
2018-10-23 10:08 manx Target Version => OpenMPT 1.28.01.00 / libopenmpt 0.4.0 (upgrade first)
2018-10-23 11:32 Saga Musix Note Added: 0003687
2018-10-23 11:32 Saga Musix Note View State: 0003685: public
2018-10-23 12:07 manx Note Added: 0003688
2018-10-23 12:10 Saga Musix Note Added: 0003689
2018-10-23 13:00 manx Note Added: 0003690
2018-10-28 20:50 Saga Musix View Status private => public
2018-10-28 20:50 Saga Musix Description Updated
2018-10-28 20:50 Saga Musix Note Added: 0003693
2018-11-03 08:16 manx File Added: file-flush-v2.patch
2018-11-04 11:25 manx File Added: file-flush-v4.patch
2018-11-04 11:25 manx Note Added: 0003696
2018-11-06 18:46 Saga Musix Note Added: 0003698
2018-11-06 18:58 manx Note Added: 0003699
2018-11-06 18:58 Saga Musix Note Added: 0003700
2018-11-06 19:16 manx Relationship added related to 0001161
2018-11-07 21:56 Saga Musix Note Added: 0003703
2018-11-08 07:05 StarWolf3000 Note Added: 0003704
2018-11-08 13:29 manx Note Added: 0003705
2018-11-08 18:15 Saga Musix Note Added: 0003706
2018-11-09 07:34 manx Status acknowledged => resolved
2018-11-09 07:34 manx Resolution open => fixed
2018-11-09 07:34 manx Fixed in Version => OpenMPT 1.28.01.00 / libopenmpt 0.4.0 (upgrade first)
2018-11-09 07:34 manx Note Added: 0003707