It looks in DC++ on Windows, the only supported DC++ platform,
FileReader::readMapped() already always fell through to
FileReader::readCached():
ret = readMapped(file, callback);
if(ret == READ_FAILED) {
dcdebug("Reading [full] %s\n", file.c_str());
ret = readCached(file, callback);
}
...
#ifdef _WIN32
...
size_t FileReader::readMapped(const string& file, const DataCallback& callback)
{
/** @todo mapped reads can fail on Windows by throwing an exception
that may only be caught by
SEH. MinGW doesn't have that, thus making this method of reading prone
to unrecoverable
failures. disabling this for now should be fine as DC++ always tries
overlapped reads first
(at the moment this file reader is only used in places where overlapped
reads make the most
sense).
more info:
<https://msdn.microsoft.com/en-us/library/aa366801(VS.85).aspx>
<https://stackoverflow.com/q/7244645> */
#if 1
return READ_FAILED;
#else
...
#endif
}
#else
...
static sigjmp_buf sb_env;
static void sigbus_handler(int signum, siginfo_t* info, void* context) {
// Jump back to the readMapped which will return error. Apparently
truncating
// a file in Solaris sets si_code to BUS_OBJERR
if (signum == SIGBUS && (info->si_code == BUS_ADRERR || info->si_code
== BUS_OBJERR))
siglongjmp(sb_env, 1);
}
size_t FileReader::readMapped(const string& filename, const DataCallback&
callback) {
...
}
#endif
Had this also been the case in AirDC++ before you disabled
FileReader::readMapped() entirely, or were you only seeing reports of it
from non-Windows systems, such as the Gentoo bug you linked?
Either way, it seems like a safe, reasonable, risk-averse change, since
DC++ only targets Windows platforms currently, so at best there's a
bunch of dead code in one platform-dependent preprocessor branch and
some and very likely subtly incorrect code in the other platform-
dependent branch. I'm inclined to remove FileReader::readMapped(),
exactly as you suggest.
--
You received this bug notification because you are a member of
Dcplusplus-team, which is subscribed to DC++.
https://bugs.launchpad.net/bugs/1909861
Title:
FileReader is not thread safe on Linux
Status in DC++:
New
Bug description:
FileReader::readMapped currently modifies the global SIGBUS handler in
order to catch read errors:
https://sourceforge.net/p/dcplusplus/code/ci/default/tree/dcpp/FileReader.cpp#l289
Since the function can be called concurrently from different threads
(currently hashing/queue recheck/sfv check in DC++) and each of them
sets and resets the SIGBUS handler, there's a high risk that the
application will crash in case of read errors as they aren't being
handler properly.
More information about the caveats:
https://www.sublimetext.com/blog/articles/use-mmap-with-care
These issues are much more likely to happen with AirDC++ as it uses
multiple threads for hashing. Read errors caused rather nasty crashes
with corrupted stack traces for one user, but luckily he was able to
catch the SIGBUS signal with gdb.
I didn't even spend time in trying to figure out how to make the
mapped reads work properly, as based on my testing the basic
FileReader::readCached function is noticeably faster:
readMapped: 671 files (21.70 GiB) in 9 directories have been hashed in 4
minutes 21 seconds (84.87 MiB/s)
readCached: 671 files (21.70 GiB) in 9 directories have been hashed in 3
minutes 58 seconds (93.08 MiB/s)
FileReader::readMapped is now disabled in AirDC++, as I can't see any
benefits from using it. The included setjmp.h header is even causing
issues when using clang for compiling on Linux:
https://bugs.gentoo.org/731676
To manage notifications about this bug go to:
https://bugs.launchpad.net/dcplusplus/+bug/1909861/+subscriptions
_______________________________________________
Mailing list: https://launchpad.net/~linuxdcpp-team
Post to : linuxdcpp-team@lists.launchpad.net
Unsubscribe : https://launchpad.net/~linuxdcpp-team
More help : https://help.launchpad.net/ListHelp
