search

DISTORTEC / distortos

object-oriented C++ RTOS for microcontrollers
https://distortos.org/
Mozilla Public License 2.0
434 stars 67 forks source link

Is this missing a `const std::lock_guard<Littlefs1File> lockGuard {*this};`? #54

Closed henrygab closed 4 years ago

henrygab commented 4 years ago

https://github.com/DISTORTEC/distortos/blob/746afae4c4d8485c9aafc572364861c6ac6de223/source/FileSystem/littlefs1/Littlefs1File.cpp#L99-L100 https://github.com/DISTORTEC/distortos/blob/746afae4c4d8485c9aafc572364861c6ac6de223/source/FileSystem/littlefs1/Littlefs1File.cpp#L128

All other functions seem to use the lock_guard<>, except for open(), and open() is calling into the LFS1 API directly. Therefore, doesn't open() also need synchronization, since LFS1 is not safe to access from multiple threads?

If this is intentional, can you help me understand why this instance is OK? (I ask this because I'm looking at how to make an existing code base that is using LFSv1.6 safe to call from multiple threads, and if open() is inherently thread-safe for some reason … well that'd be interesting.)

Thank you!

FreddieChopin commented 4 years ago

If this is intentional, can you help me understand why this instance is OK?

It is intentional and correct. The explanation may be a bit complex, but hopefully I'll be able to describe it in detail.

  1. Littlefs1File class is actually a private class, not accessible and not visible to the user of the application - you can tell this by the location of its declaration - the header is NOT in the global include/distortos/... path.
  2. The only thing a user of the application ever gets is the pointer to the abstract File class.
  3. The only way to obtain this pointer to the abstract File class is to use either pure virtual FileSystem::openFile() or any of its specific implementations directly (like Littlefs1FileSystem::openFile()).
  4. Declaration of abstract File class does NOT include any facility to open this file - there is no open() there.
  5. Therefore even though Littlefs1File::open() is public, it can only be used via Littlefs1FileSystem::openFile(), which already holds the lock https://github.com/DISTORTEC/distortos/blob/746afae4c4d8485c9aafc572364861c6ac6de223/source/FileSystem/littlefs1/Littlefs1FileSystem.cpp#L342 - this is the reason why you don't need another one.
  6. It is virtually impossible to call Littlefs1File::open() in any other way from the application.

Let me know whether this explains this puzzle for you, as I imagine this may be a bit confusing. Generally lfs1_file_open() is not any more thread safe than any other lfs function, so it also needs a lock. This lock is there, just "one level above", due to the way Littlefs1File::open() has to be used.

henrygab commented 4 years ago

It is intentional and correct. The explanation may be a bit complex, but hopefully I'll be able to describe it in detail.

@FreddieChopin -- PERFECT description, thanks! I hadn't grok'd the abstraction of the project as a whole, and what you said is quite accurate (and careful even to say "virtually impossible"). Very well explained, and great summary paragraph at the end also.

Thank you!