Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

core: implement correct fs abstraction for mobile targets #5

Closed
junr03 opened this issue Apr 18, 2019 · 5 comments
Closed

core: implement correct fs abstraction for mobile targets #5

junr03 opened this issue Apr 18, 2019 · 5 comments
Assignees
Labels
Milestone

Comments

@junr03
Copy link
Member

junr03 commented Apr 18, 2019

This issue is a subtask of #55

(It might be right that this should be reflected as an Envoy OSS issue, but going to discuss here because of the context)

Envoy OSS has a filesystem abstraction in order to manipulate the host's filesystem. The interfaces live here. A few months ago several folks from the OSS refactored the implementation of the filesystem abstraction in order to allow for platform specific implementations of the filesystem via bazel select statements like this.

The current implementations depend on APIs that don't exist or currently are not supported. Moreover, in Mobile architectures the Envoy thread would not have full access to the filesystem like it does when running on a machine in "server mode". Thus in order to be able to run tests that depend on the filesystem libraries, we need implementations that work against mobile architectures.

Note: I haven't looked into the watcher area of this problem space yet.

Where are these platform dependent libraries used in tests?

directory_lib
|_> //test/common/filesystem:directory_test
|_> //test/test_common:utility_lib

filesystem_lib
|_> //test/common/filesystem:filesystem_impl_test
|_> //test/test_common:utility_lib
|_> //test/test_common:file_system_for_test_lib
    |_> |_> //test/test_common:utility_lib

Note: it is funky that //test/test_common:utility_lib depends on both //test/test_common:file_system_for_test_lib and //test/test_common:file_system_lib. I need to look into that.

Options for a filesystem implementation that works against mobile architectures

  1. Create usable filesystem implementations against each mobile architecture. This would allow us to depend on working implementations for tests, and potentially in production code. This might be easily done in a future version of Xcode that provides a sandboxed filesystem, and with the standardized std::filesystem library in C++17. I have not done research on this for Android.
  2. Alternatively, we could create a fake filesystem implementation that builds an in memory filesystem against the API that Envoy has. This could very simply be implemented with a map of paths to simple FakeFile structs (that have a file type, a buffer, etc.). This option would be viable to run tests against both mobile architectures. However, it would obviously not be a usable model in production code.
@junr03 junr03 self-assigned this Apr 18, 2019
@junr03
Copy link
Member Author

junr03 commented Apr 18, 2019

From @mattklein123

I would try to do a real implementation if it's possible. We will need to use the FS on mobile for caching anyway. The required APIs are pretty simple so I don't think it will be too difficult I'm guessing.

@tonya11en
Copy link
Member

@junr03 your note says that //test/test_common:utility_lib depends on the same library twice. I think there's a typo there.

@junr03
Copy link
Member Author

junr03 commented Apr 19, 2019

Yep, edited. Thanks!

@junr03
Copy link
Member Author

junr03 commented Apr 19, 2019

I wrote a quick in-memory implementation of the filesystem interface yesterday. I am going to use that to move ahead with the goal of getting #55 ready. In parallel I am going to spec out the production ready, real implementation for both iOS and Android.

I agree that we want to have that concretely in the future, I just don't want to block #55 on that.

@junr03 junr03 transferred this issue from another repository May 1, 2019
@junr03 junr03 added the core label May 1, 2019
@junr03 junr03 added this to the v0.2 "Primo" milestone May 2, 2019
@stale
Copy link

stale bot commented Jun 1, 2019

This issue has been automatically marked as stale because it has not had activity in the last 30 days. It will be closed in the next 7 days unless it is tagged "help wanted" or other activity occurs. Thank you for your contributions.

@stale stale bot added the stale label Jun 1, 2019
@junr03 junr03 removed stale labels Jun 3, 2019
@junr03 junr03 closed this as completed Jun 17, 2019
jpsim added a commit that referenced this issue Apr 22, 2022
This was causing hangs/crashes in our Lyft apps so I'm reverting this
while I continue to investigate.

Here's the crash backtrace I'm seeing on iOS (not very helpful
unfortunately):

```
* thread #11, stop reason = signal SIGABRT
  * frame #0: 0x000000013a02300e libsystem_kernel.dylib`__pthread_kill + 10
    frame #1: 0x00000001399a61ff libsystem_pthread.dylib`pthread_kill + 263
    frame #2: 0x0000000138905684 libsystem_c.dylib`abort + 123
    frame #3: 0x00000001024793af Lyft`___lldb_unnamed_symbol4253$$Lyft + 3231
    frame #4: 0x000000010247e0c7 Lyft`___lldb_unnamed_symbol4339$$Lyft + 130
    frame #5: 0x00000001399a64e1 libsystem_pthread.dylib`_pthread_start + 125
    frame #6: 0x00000001399a1f6b libsystem_pthread.dylib`thread_start + 15
```

This breaks h3 functionality, which merged yesterday and is as of yet
unused.

Signed-off-by: JP Simard <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants