Add Emscripten target #6263

fealebenpae · 2023-02-01T16:31:24Z

What, How & Why?

☑️ ToDos

📝 Changelog update
🚦 Tests (or not relevant)
C-API, if public C++ API changed.

jedelbo · 2023-02-21T13:07:43Z

.gitmodules

@@ -1,3 +1,9 @@
 [submodule "external/catch"]
 	path = external/catch
 	url = https://github.com/catchorg/Catch2.git
+[submodule "external/sha-2"]


These submodules should be placed in src/external.

# Conflicts: # .gitmodules # src/realm/CMakeLists.txt # src/realm/util/encrypted_file_mapping.cpp # src/realm/util/sha_crypto.cpp # test/object-store/sync/app.cpp

michael-wb · 2023-03-27T17:57:06Z

src/realm/object-store/sync/impl/emscripten/network_transport.cpp

+    std::vector<const char*> request_headers_buf;
+    for (const auto& header : request.headers) {
+        request_headers_buf.push_back(header.first.c_str());
+        request_headers_buf.push_back(header.second.c_str());
+    }
+    request_headers_buf.push_back(nullptr);
+    attr.requestHeaders = request_headers_buf.data();


Is there any risk of request_headers_buf going out of scope before emscripten_fetch() has had a chance to use it? Or will it generate and send the request before that function returns?

emscripten_fetch copies all the request data into JavaScript memory (it's implemented in JavaScript) before returning so we don't need those buffers to last longer than the stack frame.

michael-wb · 2023-03-27T19:26:18Z

src/realm/object-store/sync/app.cpp

+#ifdef __EMSCRIPTEN__
+    if (!m_config.transport) {
+        m_config.transport = std::make_shared<_impl::EmscriptenNetworkTransport>();
+    }
+#endif


Is emscripten only going to be used with the WASM SDK? If so, what would you think about moving the transport and socketprovider classes to the SDK?

Many of our SDKs can have wasm builds - realm-js is the priority now, but realm-dotnet is sure to follow, based on requests. Dart and Kotlin can also benefit. Since the networking code is browser-specific, but multiple SDKs could target the browser I opted to have it live in Core and be the default unless an SDKs chooses to do otherwise. However, since we have little control over the browser's network stack and our customization opportunities are limited I don't know if SDKs would ever need to override the defaults, expect maybe in tests where they mock the server endpoints.

OK - we don't have to decide this now. If we decide to keep it in core since it is needed by multiple SDKs (fortunately it's a fairly lightweight set of classes), we should make sure we add tests to verify its operation. I did see that you added the emscripten Jenkins build test.

Yes - ideally I'd like to run Core's existing test suite, but right now we can only build them, not run them. The big blocker there is that virtually all of our tests expect filesystem persistence, and a lot of them rely on being able to spawn threads, both of which are unsupported right now. But as the project matures the expectation is that we'll be running Core tests on the target.

RedBeard0531 · 2023-03-27T09:49:19Z

CMakeLists.txt

@@ -132,6 +132,13 @@ if(ANDROID)
    add_compile_options(-fdata-sections -ffunction-sections -fomit-frame-pointer -fsigned-char -fstrict-aliasing -funwind-tables -no-canonical-prefixes $<$<CONFIG:Release>:-Oz>)
 endif()

+if(EMSCRIPTEN)
+    add_compile_options(-fwasm-exceptions)


It is better to use target_compiler_options(PUBLIC) on the realm target so that it automatically applies to all targets, including those of the SDKs that link against it. That said, you may need to use add_compile_options for any third_party code that doesn't depend on realm.

ditto for link_options.

That would be cumbersome because we'll need these on Storage, Bid, and a few other targets. Rather I can only set these if stand-alone Core builds (for tests) and let SDKs control the exception handling and memory growth before they include the Core submodule.

CMakeLists.txt

src/realm/CMakeLists.txt

RedBeard0531 · 2023-03-27T15:15:29Z

src/realm/object-store/sync/impl/emscripten/socket_provider.cpp

+    attr.protocols = protocols.c_str();
+    attr.url = url.c_str();
+    int result = emscripten_websocket_new(&attr);
+    REALM_ASSERT(result > 0);


We should probably have some common util function for working with EMSCRIPTEN_RESULTs so we get more info on failure, but I'm fine leaving that as a future TODO though.

This one is special - if the return value is >0 then it's a socket descriptor, if it's <0 then it's EMSCRIPTEN_RESULT. And this function only errors out if websockets are not supported, or attr is null. I'm not sure what we could do with this information, so I didn't think to record the actual error code.

Well we should at least log it and throw an exception or abort, rather than soldiering on as if nothing happened. Note that REALM_ASSERT is probably not sufficient for that because they are removed in release builds. It should only be used for things that we believe to be both impossible and within our control, and I don't think this qualifies.

Although again, I guess this code isn't 100% production ready anyway, so if you want to just file a ticket to revisit error handling of emscripten calls, and leave as-is for this PR, I'm fine with that.

This code is 100% not production ready 😅
But in this particular case I'd argue the error conditions are entirely (Realm) programmer error - we either shipped on a browser target so old it doesn't support websockets, or we didn't initialize the function arguments correctly.
Right now release builds for WebAssembly omit any useful debug information so we lose stack traces and exception types and all of that, so even an REALM_ASSERT_RELEASE doesn't give us very actionable information. That is something we need to explore more when we tune file size. That's why I'd prefer to have just enough validation for Debug builds for now.

Not disagreeing with your overall point, however

we either shipped on a browser target so old it doesn't support websockets

Isn't it the end user who chooses the browser they run on? Or do we have checks to ensure that we are using a new enough browser with sufficient support for websockets?

even an REALM_ASSERT_RELEASE doesn't give us very actionable information

Don't we still get logging of the file+line that failed? That seems like it would give us very actionable info in cases like that because we could tell the exact call that failed. More so if we used one of the macros that also printed the value of the result, eg REALM_ASSERT_3(result, >, 0) (which unfortunately doesn't have a RELEASE variant 😢)

Or do we have checks to ensure that we are using a new enough browser with sufficient support for websockets?

In our particular case the JS SDK itself won't run on a browser that old. But if I think about it some more, this will error out on Node.js as well, since that doesn't come with websockets out of the box.

Don't we still get logging of the file+line that failed?

If we put the assert inside a convenience function that checks EMSCRIPTEN_RESULT wouldn't we only get the line number of that function?

If we put the assert inside a convenience function that checks EMSCRIPTEN_RESULT wouldn't we only get the line number of that function?

There are solutions to that, including using a macro rather than a normal function. With C++20's source_location, we could use a plain function, although that requires a new compiler (only finished for clang/libc++ 16 which was released a few days ago ☹️).

src/realm/object-store/sync/impl/emscripten/socket_provider.hpp

src/realm/sync/noinst/server/server_history.cpp

src/realm/util/basic_system_errors.cpp

test/object-store/util/event_loop.cpp

RedBeard0531 · 2023-04-03T13:55:50Z

src/realm/object-store/sync/impl/emscripten/network_transport.cpp

 using namespace realm::app;

-namespace {


Why did you stop using the anonymous namespace and switch to static? One of the downsides is that you can't use static with types. I don't think that really matters in this case, but that is why namespace{} is usually preferred over static, and it may start to matter if methods are ever added to FetchState.

Apologies, I misinterpreted a previous comment about using a namespace. I will revert this change.

src/realm/object-store/sync/impl/emscripten/socket_provider.cpp

RedBeard0531 · 2023-04-03T17:01:21Z

src/realm/object-store/sync/impl/emscripten/socket_provider.cpp

+    attr.protocols = protocols.c_str();
+    attr.url = url.c_str();
+    int result = emscripten_websocket_new(&attr);
+    REALM_ASSERT(result > 0);


If we put the assert inside a convenience function that checks EMSCRIPTEN_RESULT wouldn't we only get the line number of that function?

There are solutions to that, including using a macro rather than a normal function. With C++20's source_location, we could use a plain function, although that requires a new compiler (only finished for clang/libc++ 16 which was released a few days ago ☹️).

RedBeard0531

Just a few minor nits

Co-authored-by: Mathias Stearn <[email protected]>

# Conflicts: # CMakeLists.txt

fealebenpae self-assigned this Feb 1, 2023

cla-bot bot added the cla: yes label Feb 1, 2023

fealebenpae force-pushed the yg/emscripten branch from f69c299 to 1684cfd Compare February 1, 2023 16:35

Add Emscripten target

4734cdb

fealebenpae force-pushed the yg/emscripten branch 9 times, most recently from 8fc60f6 to 1425c93 Compare February 2, 2023 23:51

refactor hmac calculation

9f26b3d

fealebenpae force-pushed the yg/emscripten branch 2 times, most recently from 916fd7b to 3419c01 Compare February 3, 2023 00:15

fix windows build

9d8f848

fealebenpae force-pushed the yg/emscripten branch from 3419c01 to 9d8f848 Compare February 3, 2023 00:39

fealebenpae added 4 commits February 3, 2023 01:50

fix scheduler

b27ea9b

fix app test

57a1dc6

fix SPM build

45d15b4

fix warnings

492f71c

fealebenpae force-pushed the yg/emscripten branch from 3275999 to 492f71c Compare February 3, 2023 13:12

fealebenpae added 2 commits February 3, 2023 21:04

add noop emscripten ExternalCommitHelper

f3c3766

disable async writes

f35c949

fealebenpae force-pushed the yg/emscripten branch from 442285a to f35c949 Compare February 3, 2023 20:10

jedelbo reviewed Feb 21, 2023

View reviewed changes

fealebenpae mentioned this pull request Mar 8, 2023

Refactor SHA hashing and the OpenSSL dependency #6308

Merged

3 tasks

Merge remote-tracking branch 'origin/master' into yg/emscripten

615ec45

# Conflicts: # .gitmodules # src/realm/CMakeLists.txt # src/realm/util/encrypted_file_mapping.cpp # src/realm/util/sha_crypto.cpp # test/object-store/sync/app.cpp

fealebenpae force-pushed the yg/emscripten branch from ae14e6e to 615ec45 Compare March 23, 2023 15:31

undo changes

11b1e62

fealebenpae requested review from michael-wb and RedBeard0531 March 27, 2023 09:07

use unique_ptr

f80f055

fealebenpae marked this pull request as ready for review March 27, 2023 09:25

michael-wb reviewed Mar 27, 2023

View reviewed changes

RedBeard0531 reviewed Mar 28, 2023

View reviewed changes

fealebenpae force-pushed the yg/emscripten branch 3 times, most recently from 614ab50 to fc9da3e Compare March 31, 2023 21:11

code review

0f6398e

fealebenpae force-pushed the yg/emscripten branch from fc9da3e to 0f6398e Compare March 31, 2023 21:16

RedBeard0531 reviewed Apr 3, 2023

View reviewed changes

RedBeard0531 approved these changes Apr 3, 2023

View reviewed changes

fealebenpae and others added 8 commits April 25, 2023 13:53

Apply suggestions from code review

ad63681

Co-authored-by: Mathias Stearn <[email protected]>

Merge remote-tracking branch 'origin/master' into yg/emscripten

aabece5

# Conflicts: # CMakeLists.txt

fix linting

98a6782

changelog

29816f6

Merge branch 'master' of github.com:realm/realm-core into yg/emscripten

db516da

fixes

552103a

lint

028dc4a

bump Emscripten on CI

73effc2

fealebenpae force-pushed the yg/emscripten branch from 631d97a to d0cbd30 Compare May 8, 2023 14:40

add ExternalCommitHelper implementation

a56b3f5

fealebenpae force-pushed the yg/emscripten branch from d0cbd30 to a56b3f5 Compare May 8, 2023 16:04

Merge branch 'master' of github.com:realm/realm-core into yg/emscripten

5c2d17d

fealebenpae merged commit 6a40e0e into master May 9, 2023

fealebenpae deleted the yg/emscripten branch May 9, 2023 10:29

cmelchior mentioned this pull request Jun 9, 2023

Kotlin JS realm/realm-kotlin#1169

Open

github-actions bot locked as resolved and limited conversation to collaborators Mar 21, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Emscripten target #6263

Add Emscripten target #6263

fealebenpae commented Feb 1, 2023 •

edited

Loading

jedelbo Feb 21, 2023

michael-wb Mar 27, 2023

fealebenpae Mar 28, 2023

michael-wb Mar 27, 2023

fealebenpae Mar 28, 2023

michael-wb Mar 28, 2023

fealebenpae Mar 28, 2023

RedBeard0531 Mar 27, 2023

fealebenpae Mar 30, 2023

RedBeard0531 Mar 27, 2023

fealebenpae Mar 31, 2023

RedBeard0531 Mar 31, 2023

fealebenpae Mar 31, 2023

RedBeard0531 Mar 31, 2023

fealebenpae Mar 31, 2023

RedBeard0531 Apr 3, 2023

RedBeard0531 Apr 3, 2023 •

edited

Loading

fealebenpae Apr 25, 2023

RedBeard0531 Apr 3, 2023

RedBeard0531 left a comment

Add Emscripten target #6263

Add Emscripten target #6263

Conversation

fealebenpae commented Feb 1, 2023 • edited Loading

What, How & Why?

☑️ ToDos

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RedBeard0531 Apr 3, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RedBeard0531 left a comment

Choose a reason for hiding this comment

fealebenpae commented Feb 1, 2023 •

edited

Loading

RedBeard0531 Apr 3, 2023 •

edited

Loading