File locking and DevelopmentService improvements (#358)

- Moved `file::getlock(path)` from `Tactility` to `TactilityCore`
- Changed all existing `file::*` functions to implement locking by default
- Removed all manual locking where `file::*` functions were used
- When `DevelopmentService` receives a file, it doesn't try to allocate it all in memory. This fixes going out-of-memory on devices without PSRAM.
- Fix for TactilityC include
This commit is contained in:
Ken Van Hoeylandt 2025-10-05 16:16:55 +02:00 committed by GitHub
parent a05a6afaaf
commit 3802679de4
No known key found for this signature in database
GPG Key ID: B5690EEEBB952194
18 changed files with 224 additions and 180 deletions

View File

@ -13,20 +13,8 @@ namespace tt::file {
/**
* @param[in] path the path to find a lock for
* @return a non-null lock for the specified path.
* @return a lock instance when a lock was found, otherwise nullptr
*/
std::shared_ptr<Lock> getLock(const std::string& path);
/**
* Acquires a lock, calls the function, then releases the lock.
* @param[in] path the path to find a lock for
* @param[in] fn the code to execute while the lock is acquired
*/
template<typename ReturnType>
ReturnType withLock(const std::string& path, std::function<ReturnType()> fn) {
const auto lock = getLock(path)->asScopedLock();
lock.lock();
return fn();
}
std::shared_ptr<Lock> _Nullable findLock(const std::string& path);
}

View File

@ -24,6 +24,8 @@ std::map<std::string, std::string> parseContentDisposition(const std::vector<std
bool readAndDiscardOrSendError(httpd_req_t* request, const std::string& toRead);
size_t receiveFile(httpd_req_t* request, size_t length, const std::string& filePath);
}
#endif // ESP_PLATFORM

View File

@ -17,6 +17,7 @@
#include <map>
#include <format>
#include <Tactility/file/FileLock.h>
#ifdef ESP_PLATFORM
#include <Tactility/InitEsp.h>
@ -188,11 +189,9 @@ static void registerInstalledApps(const std::string& path) {
static void registerInstalledAppsFromSdCard(const std::shared_ptr<hal::sdcard::SdCardDevice>& sdcard) {
auto sdcard_root_path = sdcard->getMountPath();
auto app_path = std::format("{}/app", sdcard_root_path);
sdcard->getLock()->lock();
if (file::isDirectory(app_path)) {
registerInstalledApps(app_path);
}
sdcard->getLock()->unlock();
}
static void registerInstalledAppsFromSdCards() {
@ -269,6 +268,7 @@ void run(const Configuration& config) {
#ifdef ESP_PLATFORM
initEsp();
#endif
file::setFindLockFunction(file::findLock);
settings::initTimeZone();
hal::init(*config.hardware);
network::ntp::init();

View File

@ -106,12 +106,9 @@ static bool untar(const std::string& tarPath, const std::string& destinationPath
}
void cleanupInstallDirectory(const std::string& path) {
const auto lock = file::getLock(path);
lock->lock();
if (!file::deleteRecursively(path)) {
TT_LOG_W(TAG, "Failed to delete existing installation at %s", path.c_str());
}
lock->unlock();
}
bool install(const std::string& path) {
@ -121,23 +118,18 @@ bool install(const std::string& path) {
auto app_parent_path = getAppInstallPath();
TT_LOG_I(TAG, "Installing app %s to %s", path.c_str(), app_parent_path.c_str());
auto target_path_lock = file::getLock(app_parent_path)->asScopedLock();
target_path_lock.lock();
auto filename = file::getLastPathSegment(path);
const std::string app_target_path = std::format("{}/{}", app_parent_path, filename);
if (file::isDirectory(app_target_path) && !file::deleteRecursively(app_target_path)) {
TT_LOG_W(TAG, "Failed to delete %s", app_target_path.c_str());
}
target_path_lock.unlock();
target_path_lock.lock();
if (!file::findOrCreateDirectory(app_target_path, 0777)) {
TT_LOG_I(TAG, "Failed to create directory %s", app_target_path.c_str());
return false;
}
target_path_lock.unlock();
auto target_path_lock = file::getLock(app_parent_path)->asScopedLock();
auto source_path_lock = file::getLock(path)->asScopedLock();
target_path_lock.lock();
source_path_lock.lock();
@ -149,23 +141,19 @@ bool install(const std::string& path) {
source_path_lock.unlock();
target_path_lock.unlock();
target_path_lock.lock();
auto manifest_path = app_target_path + "/manifest.properties";
if (!file::isFile(manifest_path)) {
TT_LOG_E(TAG, "Manifest not found at %s", manifest_path.c_str());
cleanupInstallDirectory(app_target_path);
return false;
}
target_path_lock.unlock();
target_path_lock.lock();
std::map<std::string, std::string> properties;
if (!file::loadPropertiesFile(manifest_path, properties)) {
TT_LOG_E(TAG, "Failed to load manifest at %s", manifest_path.c_str());
cleanupInstallDirectory(app_target_path);
return false;
}
target_path_lock.unlock();
AppManifest manifest;
if (!parseManifest(properties, manifest)) {
@ -179,7 +167,6 @@ bool install(const std::string& path) {
stopAll(manifest.appId);
}
target_path_lock.lock();
const std::string renamed_target_path = std::format("{}/{}", app_parent_path, manifest.appId);
if (file::isDirectory(renamed_target_path)) {
if (!file::deleteRecursively(renamed_target_path)) {
@ -188,15 +175,16 @@ bool install(const std::string& path) {
return false;
}
}
target_path_lock.unlock();
target_path_lock.lock();
if (rename(app_target_path.c_str(), renamed_target_path.c_str()) != 0) {
bool rename_success = rename(app_target_path.c_str(), renamed_target_path.c_str()) == 0;
target_path_lock.unlock();
if (!rename_success) {
TT_LOG_E(TAG, "Failed to rename \"%s\" to \"%s\"", app_target_path.c_str(), manifest.appId.c_str());
cleanupInstallDirectory(app_target_path);
return false;
}
target_path_lock.unlock();
manifest.appLocation = Location::external(renamed_target_path);
@ -214,7 +202,6 @@ bool uninstall(const std::string& appId) {
}
auto app_path = getAppInstallPath(appId);
return file::withLock<bool>(app_path, [&app_path, &appId] {
if (!file::isDirectory(app_path)) {
TT_LOG_E(TAG, "App %s not found at ", app_path.c_str());
return false;
@ -229,7 +216,6 @@ bool uninstall(const std::string& appId) {
}
return true;
});
}
} // namespace

View File

@ -57,7 +57,6 @@ bool State::setEntriesForPath(const std::string& path) {
return true;
} else {
dir_entries.clear();
return file::withLock<bool>(path, [this, &path] {
int count = file::scandir(path, dir_entries, &file::direntFilterDotEntries, file::direntSortAlphaAndType);
if (count >= 0) {
TT_LOG_I(TAG, "%s has %u entries", path.c_str(), count);
@ -69,7 +68,6 @@ bool State::setEntriesForPath(const std::string& path) {
TT_LOG_E(TAG, "Failed to fetch entries for %s", path.c_str());
return false;
}
});
}
}

View File

@ -317,17 +317,18 @@ void View::onResult(LaunchId launchId, Result result, std::unique_ptr<Bundle> bu
switch (state->getPendingAction()) {
case State::ActionDelete: {
if (alertdialog::getResultIndex(*bundle) == 0) {
file::withLock<void>(filepath, [&filepath] {
if (file::isDirectory(filepath)) {
if (!file::deleteRecursively(filepath)) {
TT_LOG_W(TAG, "Failed to delete %s", filepath.c_str());
}
} else if (file::isFile(filepath)) {
auto lock = file::getLock(filepath);
lock->lock();
if (remove(filepath.c_str()) <= 0) {
TT_LOG_W(TAG, "Failed to delete %s", filepath.c_str());
}
lock->unlock();
}
});
state->setEntriesForPath(state->getCurrentPath());
update();
@ -337,14 +338,15 @@ void View::onResult(LaunchId launchId, Result result, std::unique_ptr<Bundle> bu
case State::ActionRename: {
auto new_name = inputdialog::getResult(*bundle);
if (!new_name.empty() && new_name != state->getSelectedChildEntry()) {
file::withLock<void>(filepath, [this, &filepath, &new_name] {
auto lock = file::getLock(filepath);
lock->lock();
std::string rename_to = file::getChildPath(state->getCurrentPath(), new_name);
if (rename(filepath.c_str(), rename_to.c_str())) {
TT_LOG_I(TAG, "Renamed \"%s\" to \"%s\"", filepath.c_str(), rename_to.c_str());
} else {
TT_LOG_E(TAG, "Failed to rename \"%s\" to \"%s\"", filepath.c_str(), rename_to.c_str());
}
});
lock->unlock();
state->setEntriesForPath(state->getCurrentPath());
update();

View File

@ -34,12 +34,6 @@ std::string State::getSelectedChildPath() const {
}
bool State::setEntriesForPath(const std::string& path) {
auto lock = mutex.asScopedLock();
if (!lock.lock(100)) {
TT_LOG_E(TAG, LOG_MESSAGE_MUTEX_LOCK_FAILED_FMT, "setEntriesForPath");
return false;
}
TT_LOG_I(TAG, "Changing path: %s -> %s", current_path.c_str(), path.c_str());
/**

View File

@ -5,20 +5,8 @@
namespace tt::file {
class NoLock : public Lock {
bool lock(TickType_t timeout) const override { return true; }
bool unlock() const override { return true; }
};
static std::shared_ptr<Lock> noLock = std::make_shared<NoLock>();
std::shared_ptr<Lock> getLock(const std::string& path) {
auto sdcard_lock = hal::sdcard::findSdCardLock(path);
if (sdcard_lock != nullptr) {
return sdcard_lock;
} else {
return noLock;
}
std::shared_ptr<Lock> _Nullable findLock(const std::string& path) {
return hal::sdcard::findSdCardLock(path);
}
}

View File

@ -19,7 +19,6 @@ bool getKeyValuePair(const std::string& input, std::string& key, std::string& va
}
bool loadPropertiesFile(const std::string& filePath, std::function<void(const std::string& key, const std::string& value)> callback) {
return file::withLock<bool>(filePath, [&filePath, &callback] {
TT_LOG_I(TAG, "Reading properties file %s", filePath.c_str());
uint16_t line_count = 0;
std::string key_prefix = "";
@ -41,7 +40,6 @@ bool loadPropertiesFile(const std::string& filePath, std::function<void(const st
}
}
});
});
}
bool loadPropertiesFile(const std::string& filePath, std::map<std::string, std::string>& outProperties) {

View File

@ -64,11 +64,9 @@ bool TextResources::load() {
return false;
}
file::withLock<void>(file_path, [&file_path, &new_data] {
file::readLines(file_path, true, [&new_data](const char* line) {
new_data.push_back(line);
});
});
if (new_data.empty()) {
TT_LOG_E(TAG, "Couldn't find i18n data for %s", path.c_str());

View File

@ -5,6 +5,7 @@
#include <sstream>
#include <Tactility/Log.h>
#include <Tactility/StringUtils.h>
#include <Tactility/file/File.h>
#ifdef ESP_PLATFORM
@ -161,6 +162,39 @@ bool readAndDiscardOrSendError(httpd_req_t* request, const std::string& toRead)
return true;
}
size_t receiveFile(httpd_req_t* request, size_t length, const std::string& filePath) {
constexpr auto BUFFER_SIZE = 512;
char buffer[BUFFER_SIZE];
size_t bytes_received = 0;
auto lock = file::getLock(filePath)->asScopedLock();
lock.lock();
auto* file = fopen(filePath.c_str(), "wb");
if (file == nullptr) {
TT_LOG_E(TAG, "Failed to open file for writing: %s", filePath.c_str());
return 0;
}
while (bytes_received < length) {
auto expected_chunk_size = std::min<size_t>(BUFFER_SIZE, length - bytes_received);
size_t receive_chunk_size = httpd_req_recv(request, buffer, expected_chunk_size);
if (receive_chunk_size <= 0) {
TT_LOG_E(TAG, "Receive failed");
break;
}
if (fwrite(buffer, 1, receive_chunk_size, file) != receive_chunk_size) {
TT_LOG_E(TAG, "Failed to write all bytes");
break;
}
bytes_received += receive_chunk_size;
}
// Write file
fclose(file);
return bytes_received;
}
}
#endif // ESP_PLATFORM

View File

@ -243,45 +243,30 @@ esp_err_t DevelopmentService::handleAppInstall(httpd_req_t* request) {
return ESP_FAIL;
}
// Receive file
size_t content_read;
auto part_after_file = std::format("\r\n--{}--\r\n", boundary);
auto file_size = content_left - part_after_file.length();
auto buffer = network::receiveByteArray(request, file_size, content_read);
if (content_read != file_size) {
httpd_resp_send_err(request, HTTPD_400_BAD_REQUEST, "Multipart form error: file data not received");
return ESP_FAIL;
}
content_left -= content_read;
// Receive boundary
auto boundary_and_newlines_after_file = std::format("\r\n--{}--\r\n", boundary);
auto file_size = content_left - boundary_and_newlines_after_file.length();
// Create tmp directory
const std::string tmp_path = getTempPath();
auto lock = file::getLock(tmp_path)->asScopedLock();
lock.lock();
if (!file::findOrCreateDirectory(tmp_path, 0777)) {
httpd_resp_send_err(request, HTTPD_500_INTERNAL_SERVER_ERROR, "Failed to save file");
return ESP_FAIL;
}
lock.unlock();
// Write file
lock.lock();
auto file_path = std::format("{}/{}", tmp_path, filename_entry->second);
auto* file = fopen(file_path.c_str(), "wb");
auto file_bytes_written = fwrite(buffer.get(), 1, file_size, file);
fclose(file);
if (file_bytes_written != file_size) {
if (network::receiveFile(request, file_size, file_path) != file_size) {
httpd_resp_send_err(request, HTTPD_500_INTERNAL_SERVER_ERROR, "Failed to save file");
return ESP_FAIL;
}
lock.unlock();
content_left -= file_size;
// Read and verify part
if (!network::readAndDiscardOrSendError(request, part_after_file)) {
if (!network::readAndDiscardOrSendError(request, boundary_and_newlines_after_file)) {
return ESP_FAIL;
}
content_left -= part_after_file.length();
content_left -= boundary_and_newlines_after_file.length();
if (content_left != 0) {
TT_LOG_W(TAG, "We have more bytes at the end of the request parsing?!");
@ -292,11 +277,9 @@ esp_err_t DevelopmentService::handleAppInstall(httpd_req_t* request) {
return ESP_FAIL;
}
lock.lock();
if (remove(file_path.c_str()) != 0) {
if (!file::deleteFile(file_path.c_str())) {
TT_LOG_W(TAG, "Failed to delete %s", file_path.c_str());
}
lock.unlock();
TT_LOG_I(TAG, "[200] /app/install -> %s", file_path.c_str());

View File

@ -82,8 +82,6 @@ static void importWifiAp(const std::string& filePath) {
static void importWifiApSettings(std::shared_ptr<hal::sdcard::SdCardDevice> sdcard) {
auto path = file::getChildPath(sdcard->getMountPath(), "settings");
auto lock = sdcard->getLock()->asScopedLock();
lock.lock();
std::vector<dirent> dirent_list;
if (file::scandir(path, dirent_list, [](const dirent* entry) {
switch (entry->d_type) {
@ -104,7 +102,6 @@ static void importWifiApSettings(std::shared_ptr<hal::sdcard::SdCardDevice> sdca
}, nullptr) == 0) {
return;
}
lock.unlock();
if (dirent_list.empty()) {
TT_LOG_W(TAG, "No AP files found at %s", sdcard->getMountPath().c_str());

View File

@ -6,6 +6,7 @@
#include <Tactility/settings/SystemSettings.h>
#include <format>
#include <Tactility/file/File.h>
namespace tt::settings {
@ -20,9 +21,7 @@ static bool loadSystemSettingsFromFile(SystemSettings& properties) {
auto file_path = std::format(FILE_PATH_FORMAT, file::MOUNT_POINT_DATA);
TT_LOG_I(TAG, "System settings loading from %s", file_path.c_str());
std::map<std::string, std::string> map;
if (!file::withLock<bool>(file_path, [&map, &file_path] {
return file::loadPropertiesFile(file_path, map);
})) {
if (!file::loadPropertiesFile(file_path, map)) {
TT_LOG_E(TAG, "Failed to load %s", file_path.c_str());
return false;
}
@ -46,9 +45,6 @@ static bool loadSystemSettingsFromFile(SystemSettings& properties) {
}
bool loadSystemSettings(SystemSettings& properties) {
auto scoped_lock = mutex.asScopedLock();
scoped_lock.lock();
if (!cached) {
if (!loadSystemSettingsFromFile(cachedSettings)) {
return false;
@ -61,11 +57,7 @@ bool loadSystemSettings(SystemSettings& properties) {
}
bool saveSystemSettings(const SystemSettings& properties) {
auto scoped_lock = mutex.asScopedLock();
scoped_lock.lock();
auto file_path = std::format(FILE_PATH_FORMAT, file::MOUNT_POINT_DATA);
return file::withLock<bool>(file_path, [&properties, &file_path] {
std::map<std::string, std::string> map;
map["language"] = toString(properties.language);
map["timeFormat24h"] = properties.timeFormat24h ? "true" : "false";
@ -78,7 +70,6 @@ bool saveSystemSettings(const SystemSettings& properties) {
cachedSettings = properties;
cached = true;
return true;
});
}
}

View File

@ -1,6 +1,6 @@
#pragma once
#include <cstdio>
#include <stdio.h>
#include "tt_app_manifest.h"

View File

@ -1,6 +1,6 @@
#include <tt_file.h>
#include <tt_lock_private.h>
#include <Tactility/file/FileLock.h>
#include <Tactility/file/File.h>
extern "C" {

View File

@ -1,3 +1,7 @@
/**
* All functions in this file can be safely called without manually applying file locks.
* For calls to C stdlib APIs such as fopen(), always call file::getLock(path) first!
*/
#pragma once
#include "Tactility/TactilityCore.h"
@ -40,6 +44,28 @@ struct FileCloser {
}
};
typedef std::function<std::shared_ptr<Lock>(const std::string&)> FindLockFunction;
/**
* @param[in] path the path to get a lock for
* @return a lock instance (never null)
*/
std::shared_ptr<Lock> getLock(const std::string& path);
void setFindLockFunction(const FindLockFunction& function);
/**
* Acquires a lock, calls the function, then releases the lock.
* @param[in] path the path to find a lock for
* @param[in] fn the code to execute while the lock is acquired
*/
template<typename ReturnType>
ReturnType withLock(const std::string& path, std::function<ReturnType()> fn) {
const auto lock = getLock(path)->asScopedLock();
lock.lock();
return fn();
}
long getSize(FILE* file);
/** Read a file and return its data.
@ -73,6 +99,10 @@ bool findOrCreateParentDirectory(const std::string& path, mode_t mode);
bool deleteRecursively(const std::string& path);
bool deleteFile(const std::string& path);
bool deleteDirectory(const std::string& path);
/**
* Concatenate a child path with a parent path, ensuring proper slash inbetween
* @param basePath an absolute path with or without trailing "/"

View File

@ -11,7 +11,33 @@ class SdCardDevice;
namespace tt::file {
#define TAG "file"
constexpr auto* TAG = "file";
class NoLock final : public Lock {
bool lock(TickType_t timeout) const override { return true; }
bool unlock() const override { return true; }
};
static std::shared_ptr<Lock> noLock = std::make_shared<NoLock>();
static std::function<std::shared_ptr<Lock>(const std::string&)> findLockFunction = nullptr;
std::shared_ptr<Lock> getLock(const std::string& path) {
if (findLockFunction == nullptr) {
TT_LOG_W(TAG, "File lock function not set!");
return noLock;
}
auto lock = findLockFunction(path);
if (lock == nullptr) {
return noLock;
}
return lock;
}
void setFindLockFunction(const FindLockFunction& function) {
findLockFunction = function;
}
std::string getChildPath(const std::string& basePath, const std::string& childPath) {
// Postfix with "/" when the current path isn't "/"
@ -40,6 +66,9 @@ bool listDirectory(
const std::string& path,
std::function<void(const dirent&)> onEntry
) {
auto lock = getLock(path)->asScopedLock();
lock.lock();
TT_LOG_I(TAG, "listDir start %s", path.c_str());
DIR* dir = opendir(path.c_str());
if (dir == nullptr) {
@ -64,6 +93,9 @@ int scandir(
ScandirFilter _Nullable filterMethod,
ScandirSort _Nullable sortMethod
) {
auto lock = getLock(path)->asScopedLock();
lock.lock();
TT_LOG_I(TAG, "scandir start");
DIR* dir = opendir(path.c_str());
if (dir == nullptr) {
@ -187,6 +219,9 @@ bool writeString(const std::string& filepath, const std::string& content) {
}
static bool findOrCreateDirectoryInternal(std::string path, mode_t mode) {
auto lock = getLock(path)->asScopedLock();
lock.lock();
struct stat dir_stat;
if (mkdir(path.c_str(), mode) == 0) {
return true;
@ -277,6 +312,7 @@ bool deleteRecursively(const std::string& path) {
TT_LOG_E(TAG, "Failed to scan directory %s", path.c_str());
return false;
}
for (const auto& entry : entries) {
auto child_path = path + "/" + entry.d_name;
if (!deleteRecursively(child_path)) {
@ -284,10 +320,10 @@ bool deleteRecursively(const std::string& path) {
}
}
TT_LOG_I(TAG, "Deleting %s", path.c_str());
return rmdir(path.c_str()) == 0;
return deleteDirectory(path);
} else if (isFile(path)) {
TT_LOG_I(TAG, "Deleting %s", path.c_str());
return remove(path.c_str()) == 0;
return deleteFile(path);
} else if (path == "/" || path == "." || path == "..") {
// No-op
return true;
@ -297,17 +333,36 @@ bool deleteRecursively(const std::string& path) {
}
}
bool deleteFile(const std::string& path) {
auto lock = getLock(path)->asScopedLock();
lock.lock();
return remove(path.c_str()) == 0;
}
bool deleteDirectory(const std::string& path) {
auto lock = getLock(path)->asScopedLock();
lock.lock();
return rmdir(path.c_str()) == 0;
}
bool isFile(const std::string& path) {
auto lock = getLock(path)->asScopedLock();
lock.lock();
return access(path.c_str(), F_OK) == 0;
}
bool isDirectory(const std::string& path) {
auto lock = getLock(path)->asScopedLock();
lock.lock();
struct stat stat_result;
return stat(path.c_str(), &stat_result) == 0 && S_ISDIR(stat_result.st_mode);
}
bool readLines(const std::string& filepath, bool stripNewLine, std::function<void(const char* line)> callback) {
auto* file = fopen(filepath.c_str(), "r");
bool readLines(const std::string& filePath, bool stripNewLine, std::function<void(const char* line)> callback) {
auto lock = getLock(filePath)->asScopedLock();
lock.lock();
auto* file = fopen(filePath.c_str(), "r");
if (file == nullptr) {
return false;
}