New low-level proxying C API #15737

tlively · 2021-12-10T06:35:11Z

Implement a new proxying API that is meant to be suitable both for proxying in
syscall implementations as well as for proxying arbitrary user work. Since the
system proxying queue is processed from so many locations, it is dangerous to
use it for arbitrary work that might take a lock or otherwise block and the work
sent to it has to be structured more like native signal handlers. To avoid this
limitation, the new API allows users to create their own proxying queues that
are processed only when the target thread returns to the JS event loop or when
the user explicitly requests processing.

In contrast to the existing proxying API, this new API:

Never drops tasks (except in the case of allocation failure). It grows the
task queues as necessary instead.
Does not attempt to dynamically type or dispatch queued functions, but rather
uses statically typed function pointers that take void* arguments. This
simplifies both the API and implementation. Packing of varargs into
dynamically typed function wrappers could easily be layered on top of this
API.
Is less redundant. There is only one way to proxy work synchronously or
asynchronously to any thread.
Is more general. It allows waiting for a task to be explicitly signaled as
done in addition to waiting for the proxied function to return.
Uses arrays instead of linked lists for better data locality.
Has a more uniform naming convention.

A follow-up PR will reimplement the existing proxying API in terms of this new
API.

tlively · 2021-12-10T06:40:11Z

cc @kripken, @sbc, @juj. This code builds, but is completely untested. Initial comments on the API and implementation are welcome. I plan to add a test of the public API, but I would like to add unit tests for the various internal components of the implementation as well, specifically the logic for growing and allocating new task queues. Does anyone have a good idea of how to do that?

sbc100

Looking very nice!

I wonder if this is good time to try to introduce the idea of internal headers to emscripten so that this could be landed without any user having access to it?

We could then build higher level stuff on top of it and polish it before making it public. Might be as simple as putting the header alongside the source code.

Of course it would still be good to be able to write tests against it so the test code might have the jump through some hoops to get this header on its include path.. but I think that is OK.

sbc100 · 2021-12-10T15:40:43Z

system/include/emscripten/proxying.h

+// nonblocking and safe to run at any time, similar to a native signal handler.
+em_proxying_queue* emscripten_proxy_get_system_queue();
+
+// Execute all the tasks enqueued for the current thread on the given queue.


Maybe say something about what happens if new tasks are queued while running the tasks.

sbc100 · 2021-12-10T15:43:40Z

system/include/emscripten/proxying.h

+int emscripten_proxy_async(em_proxying_queue* q,
+                           pthread_t target_thread,
+                           void (*func)(void*),
+                           void* arg);


What if I want to be notified when the work is done? I guess I would use a condition variable at a higher level?

Yes, that's what I would expect.

sbc100 · 2021-12-10T15:49:11Z

system/lib/pthread/proxying.c

+  if (task_queue_full(tasks)) {
+    // Allocate a larger task queue.
+    int new_capacity = tasks->capacity * 2;
+    task* new_tasks = malloc(sizeof(task) * new_capacity);


Can we use realloc here to avoid malloc + memcpy + free pattern?

Yes, but only for the case where the queue doesn't wrap around the end of the buffer. Since we need to malloc + memcpy + free on one code path anyway, it's simpler to just do it for both paths and share all the code except for the memcpy.

sbc100 · 2021-12-10T15:51:02Z

system/lib/pthread/proxying.c

+static int task_queue_enqueue(task_queue* tasks, task t) {
+  if (task_queue_full(tasks)) {
+    // Allocate a larger task queue.
+    int new_capacity = tasks->capacity * 2;


Can you split out another function here:

// Attempt to double the size of the queue int new_capacity = tasks->capacity * 2; if (!task_queue_grow(tasks, new_capacity) return 0;

sbc100 · 2021-12-10T15:54:27Z

system/lib/pthread/proxying.c

+  int tail;
+} task_queue;
+
+static int task_queue_empty(task_queue* tasks) {


How about adding // Pre-condition: em_task_queue mutex is held by calling thread here and in the following two functions.

Oh I see you have used the // Not thread safe. convention below.. maybe use there here too?

sbc100 · 2021-12-10T15:57:49Z

system/lib/pthread/proxying.c

+  if (q == NULL) {
+    return NULL;
+  }
+  pthread_mutex_init(&q->mutex, NULL);


q->mutex = PTHREAD_MUTEX_INITIALIZER works too and I think is a little more readable

In that case we can simplify this initialization to be just a single expression.

sbc100 · 2021-12-10T16:01:41Z

system/lib/pthread/proxying.c

+  assert(q != NULL);
+  task_queue* tasks = NULL;
+  for (int i = 0; i < q->size; i++) {
+    if (pthread_equal(q->task_queues[i].thread, thread)) {


Why not use TLS here to avoid the linear search? I.e. can't task_queues be accessed by pthread_get_specific?

That would work for looking up the queue for the current thread, but is there a way to make that work for looking up the queue for another thread? I guess we could have both TLS and the array, but that seems more complicated for unclear gain. I expect the linear search to very fast anyway, since I don't imagine there would ever be more than a small handful of target threads.

sbc100 · 2021-12-10T16:03:23Z

system/lib/pthread/proxying.c

+  if (pthread_equal(thread, EM_CALLBACK_THREAD_CONTEXT_CALLING_THREAD)) {
+    return pthread_self();
+  }
+  return thread;


Is this needed here? Maybe we can limit the use of these macros to the high layers?

I'm slightly worried that this will be a footgun for users, but I'm not sure how often these macros are used in the first place. I think you're right that keeping these out of the low-level API makes sense. FWIW, I decided against using EMSCRIPTEN_RESULT_{SUCCESS,FAILURE} here for largely the same reason; those macros also come from html5.h and seemed better suited for higher-level APIs.

sbc100 · 2021-12-10T16:05:08Z

system/lib/pthread/proxying.c

+                           pthread_t target_thread,
+                           void (*func)(void*),
+                           void* arg) {
+  if (q == NULL) {


Why not declare this undefined behaviour and just assert(q) here? (I think this is what musl does everywhere else for example pthread_join(0) of fread from the NULL file handle.

sbc100 · 2021-12-10T16:05:53Z

system/lib/pthread/proxying.c

+    return 0;
+  }
+  int empty = task_queue_empty(tasks);
+  if (!task_queue_enqueue(tasks, (task){func, arg})) {


goto failed and put the unlock and return at the end of the function?

Implement a new proxying API that is meant to be suitable both for proxying in syscall implementations as well as for proxying arbitrary user work. Since the system proxying queue is processed from so many locations, it is dangerous to use it for arbitrary work that might take a lock or otherwise block and the work sent to it has to be structured more like native signal handlers. To avoid this limitation, the new API allows users to create their own proxying queues that are processed only when the target thread returns to the JS event loop or when the user explicitly requests processing. In contrast to the existing proxying API, this new API: - Never drops tasks (except in the case of allocation failure). It grows the task queues as necessary instead. - Does not attempt to dynamically type or dispatch queued functions, but rather uses statically typed function pointers that take `void*` arguments. This simplifies both the API and implementation. Packing of varargs into dynamically typed function wrappers could easily be layered on top of this API. - Is less redundant. There is only one way to proxy work synchronously or asynchronously to any thread. - Is more general. It allows waiting for a task to be explicitly signaled as done in addition to waiting for the proxied function to return. - Uses arrays instead of linked lists for better data locality. - Has a more uniform naming convention. A follow-up PR will reimplement the existing proxying API in terms of this new API.

tlively · 2021-12-14T06:03:33Z

@sbc100 I've addressed all your comments, done some additional functional decomposition, and added a test that shows that proxying and the queue growth all works as expected.

The next step will be to get the postMessage notifications working. I had to comment out the code that calls _emscripten_notify_thread_queue for now because otherwise the test fails with this error: exiting due to exception: ReferenceError: postMessage is not defined. Do you know what could be going wrong there?

tlively · 2021-12-21T04:11:49Z

This is ready for proper review now. The test covers the full API, both when the target thread is explicitly executing queued tasks and when the target thread is idling on the JS event loop. Additionally, the tests exercise the logic for adding new task queues (even while actively executing tasks, which used to be a buggy case) and for growing individual task queues.

sbc100 · 2021-12-21T17:35:47Z

system/lib/pthread/proxying.c

+  if (task_buffer == NULL) {
+    return 0;
+  }
+  *tasks = (task_queue){.thread = thread,


Do you need the (task_queue) cast here?

Yes, otherwise I get an error: expected expression. This is a necessary part of the compound literal syntax.

sbc100 · 2021-12-21T17:39:37Z

system/lib/pthread/proxying.c

+}
+
+// Not thread safe.
+static task task_queue_dequeue(task_queue* tasks) {


Do we need a check here for queue being empty? Or is that a precondition at all the call sites?

It's a precondition at the call site. I'll add that to the comment.

sbc100 · 2021-12-21T17:40:34Z

system/lib/pthread/proxying.c

+  if (q == NULL) {
+    return NULL;
+  }
+  *q = (em_proxying_queue){.mutex = PTHREAD_MUTEX_INITIALIZER,


cast needed?

Yes, this is another compound literal expression.

Interesting.. I don't remember using the casts when I've used that syntax in the past... seems like the compiler should be able know.

Like for example in this case there is no cast needed:

typedef struct { int a; int b; } Foo; Foo f = {.a = 1, .b = 2};

Yeah, that should be simple type inference since C doesn't have any sort of struct subtyping. I'm not sure what the rationale for requiring the type to be repeated is.

sbc100 · 2021-12-21T17:45:38Z

tests/test_core.py

+    self.set_setting('EXIT_RUNTIME')
+    self.set_setting('PROXY_TO_PTHREAD')
+    self.set_setting('PTHREAD_POOL_SIZE=3')
+    self.set_setting('INITIAL_MEMORY=32mb')


Do you need 32mb here?

I need something larger than the default, and I arbitrarily chose 32mb as something that worked. It seemed fishy that the test was using so much memory, but instrumenting dlmalloc to print allocation sizes and stack traces didn't produce anything unexpected. By far the largest allocations are for the the pthread stacks, but those are all properly freed once the threads are joined AFAICT.

Out default pthread stack size is 2mb, which is way to large IMHO, but also you only have 3 threads here so I can't see how that would require this much memory. lgtm either to this though.. trying to reduce memory overhead is not what this change is about.

sbc100 · 2021-12-21T17:47:23Z

tests/test_core.py

+    self.set_setting('PROXY_TO_PTHREAD')
+    self.set_setting('PTHREAD_POOL_SIZE=3')
+    self.set_setting('INITIAL_MEMORY=32mb')
+    self.set_setting('EXPORTED_FUNCTIONS=_emscripten_proxy_execute_queue,_main')


If you mark _emscripten_proxy_execute_queue as EMSCRIPTEN_KEEPALIVE I think it will be exported if and only if proxying.o is included, which seems like what you want (then you can delete this line I think).

Perhaps add a comment saying Exporting so it can be called by worker.js.

I will make that minimal change, but I wonder whether the other functions should be similarly exported since they're designed to be called on arbitrary threads as well.

The "exporting" we are talking about is only needed for the case when external JS code (e.g. user code, or worker.js) needs to call a function directly.

Oh I see. Should we add it to the list here then? https://github.com/emscripten-core/emscripten/blob/main/emcc.py#L1971-L1982

We could.. but adding EMSCRIPTEN_KEEPALIVE the the source code would allow it to be only exported if the object file is used.. which would avoid paying the cost in all pthread-based programs.

Even better you can do if hasExportedFunction('_emscripten_proxy_execute_queue') in the library code (and the worker.js code) to avoid included the message handlers unless they are needed.

sbc100 · 2021-12-21T17:48:08Z

tests/test_core.py

+  def test_pthread_proxying(self):
+    self.set_setting('EXIT_RUNTIME')
+    self.set_setting('PROXY_TO_PTHREAD')
+    self.set_setting('PTHREAD_POOL_SIZE=3')


I think only one of PROXY_TO_PTHREAD and PTHREAD_POOL_SIZE should be needed (i.e. you don't need a pool if you are proxying your main).

sbc100 · 2021-12-21T17:48:35Z

tests/pthread/test_pthread_proxying.c

+  test_proxying_queue_growth();
+
+  printf("done\n");
+  emscripten_force_exit(0);


Just return 0 should be enough I think?

It turns out that using return 0 would cause the test to hang.

Thats is probably worth investigating... but we don't need to do that as part of this PR.

sbc100 · 2021-12-21T17:56:41Z

system/lib/pthread/proxying.c

+  task_queue* tasks = tasks_index == -1 ? NULL : &q->task_queues[tasks_index];
+  if (tasks == NULL || tasks->processing) {
+    // No tasks for this thread or they are already being processed.
+    pthread_mutex_unlock(&q->mutex);


Given that tasks are dequeud before being run that mutex is unlocked.. why not allow for recursive running of the queue? It certainly seems safe enough.

If we are going to fail due to recursive call perhaps we should return an error code so that we can detect that case when we have queued items but we are refusing the run them.

The old proxying API prevents recursive queue processing, which makes sense given how frequently it is processed, although I don't whether it's necessary or not. Since we want to rewrite the old API in terms of the new one, I think it makes sense to be conservative and match the old behavior here, then see what happens if we try to relax it by allowing recursive processing in the future. Another option would be to make the system queue special in the new API and allow recursive processing of all queues except the system queue, but I would rather not special case the system queue like that.

I'll add a TODO to investigate this, though.

What about leaving out this logic and adding it instead to the old API when it calls into this one? (I guess it could be as simple as a CAS with a is_processing global?). Would make this API slightly simpler and lower level which seems in line with what you are going for maybe?

Unfortunately this can't be an entirely separate layer. As part of the rewrite of the old API I would like to remove the postMessage handler for the old system and have everything go through the new handler. But the new handler would then need to call into a wrapper function for handling the recursion avoidance logic specific to the system queue. Since this recursion avoidance can't be entirely layered on top of the new system, it seems better to treat it uniformly across all queues. WDYT?

I think it's possible that removing the recursion guard in a follow-up might Just Work, though. Here's the relevant comment from the old system:

// It is possible that when processing a queued call, the control flow leads back to calling this
// function in a nested fashion! Therefore this scenario must explicitly be detected, and
// processing the queue must be avoided if we are nesting, or otherwise the same queued calls
// would be processed again and again.

You're right that the new system is different in that it dequeues before calling, so it won't run into this specific problem. I wouldn't be surprised if there are other reasons we currently depend on this behavior, so I would rather investigate as a follow-up.

Yes, I even had a PR up for while to remove the nesting limitation.. but I agree there could be other semantic reasons why recursive running is not desirable. I can't think of them.. but I suspect they exist.

lgtm to this change with or without the recursion restriction (perhaps mention in the docs that recursion doesn't currently work, and return failure when we fail to run for this reason).

tests/pthread/test_pthread_proxying.c

sbc100 · 2021-12-21T19:32:09Z

My WIP PR that I didn't really expect to land that removes the recursion/nesting limitation on the task queue runner: #15190

juj

This looks great!

One thing I am uncertain is how this relates to the original proxy queue code - looks like the intent is to replace it, but then again, this code does not change any of that, but duplicates(?) a new system queue?

juj · 2022-01-04T13:25:02Z

src/library_pthread.js

@@ -1197,6 +1197,25 @@ var LibraryPThread = {
      worker.postMessage({'cmd' : 'processThreadQueue'});
    }
    return 1;
+  },
+
+  _emscripten_notify_proxying_queue: function(targetThreadId, currThreadId, mainThreadId, queue) {


Can we locate this to a .js file of its own, e.g. library_proxy_queue.js?

That sounds nice. I'll look into it.

It turned out to be pretty simple to move this into its own JS file, but given that the implementation uses internal details of the pthread library (e.g. PThreads.pthreads[targetThreadId] and pthread.worker, and given that the existing _emscripten_notify_thread_queue will be removed in the next PR, I'm not sure there's much benefit to this after all.

I think its small enough and closely coupled enough to belong here... and as you say we hope it will replace scripten_notify_thread_queue which is already here.

juj · 2022-01-04T13:27:07Z

src/worker.js

+    } else if (e.data.cmd === 'processProxyingQueue') {
+      if (Module['_pthread_self']()) { // If this thread is actually running?
+        Module['_emscripten_proxy_execute_queue'](e.data.queue);
+      }


This part won't DCE away if user does not utilize this proxying queue.

This might be worth adding a #if LIBRARY_PROXY_QUEUE preprocessor that gets enabled when the user is targeting the new proxy queue mechanism.

IIUC, there wouldn't be any benefit to having that macro once we start using this for the system queue as well, right?

I think you can wrap this in hasExportedFunction('__emscripten_proxy_execute_queue') which will have the effect of only including it when needed. (note the double underscores)

juj · 2022-01-04T13:29:50Z

system/lib/pthread/proxying.c

+  // The target thread for this task_queue.
+  pthread_t thread;
+  // Recursion guard. TODO: We disallow recursive processing because that's what
+  // the old proxying API does. Experiment with relaxing this restriction.


This comment reads odd. The sentence We disallow recursive processing because that's what the old proxying API does suggests like recursion is disallowed because the old queue did it (and it was undesired?), but then the second sentence reads like this wants to enable that as a feature?

Or does the comment want to say that "recursive processing is disallowed here, and it is not a problem since for such use cases, the old proxying API can be used instead?"

I want to match the old behavior for now to make switching the system queue to the new queue implementation as much of a no-op as possible. Once that switch is done, it might be nice to allow recursive processing just to simplify the system. I'll make the comment more detailed.

juj · 2022-01-04T13:30:32Z

system/lib/pthread/proxying.c

+  return 1;
+}
+
+static void task_queue_deinit(task_queue* tasks) { free(tasks->tasks); }


task_queue_free()? (feel free to ignore suggestion)

I would like to keep deinit because it is more clearly pairs with init and because it's not tasks that's being freed, but rather one of its members.

juj · 2022-01-04T13:31:32Z

system/lib/pthread/proxying.c

+static void task_queue_deinit(task_queue* tasks) { free(tasks->tasks); }
+
+// Not thread safe.
+static int task_queue_empty(task_queue* tasks) {


task_queue_is_empty()? (feel free to ignore suggestion - I always disliked vector::empty() since people can often read its meaning as a verb, e.g. "empty out the trash")

juj · 2022-01-04T13:39:08Z

system/lib/pthread/proxying.h

+// Get the queue used for proxying low-level runtime work. Work on this queue
+// may be processed at any time inside system functions, so it must be
+// nonblocking and safe to run at any time, similar to a native signal handler.
+em_proxying_queue* emscripten_proxy_get_system_queue();


I interpreted this PR first to offer a user level mechanism for proxying queues? But this function reads like it is intending to supplant the original proxying queue code?

In addition to offering user-level queues, the intention is to reimplement the existing API to use this new queue implementation in a follow-up PR, mostly just to reduce the number of queue implementations we have.

juj · 2022-01-04T13:40:57Z

system/lib/pthread/proxying.h

+// Execute all the tasks enqueued for the current thread on the given queue.
+// Exported for use in worker.js.
+EMSCRIPTEN_KEEPALIVE
+void emscripten_proxy_execute_queue(em_proxying_queue* q);


Does this get emitted to all builds, even if user does not use this proxying queue? This should get gated behind a #if LIBRARY_PROXY_QUEUE if so.

The way I was suggesting this should work is that if any function from system/lib/pthread/proxying.c is included in the link then this symbol will be exported for JS so the JS can call it (that is how EMSCRIPTEN_KEEPALIVE works).

If there is no dependency on proxying.c at all then nothing should be exported. This way the JS code and use hasExportedFunction('emscripten_proxy_execute_queue') and know that it will be exported if and only if the program makes some use of proxying.

@tlively can you put EMSCRIPTEN_KEEPALIVE instead on the defintion in the C file? (or does that not work?)

Yes, I can make that change.

tlively · 2022-01-04T17:45:55Z

It looks like the new test_pthread_proxying sometimes hangs when run with asan, so I'll investigate that before landing this.

I was also working on the follow up PR to use the new queue implementation for the system queue and had everything working except for similar hangs in test_pthread_proxying, so it's probably the same underlying problem.

Rewrite the old threading.h proxying API used for internal system call implementations in terms of the new proxying API introduced in #15737.

sbc100 reviewed Dec 10, 2021

View reviewed changes

tlively added 11 commits December 12, 2021 21:35

Remove normalize_thread

99f14f5

Make header internal

59a7bb3

Assert that q is not null

faa6423

Simplify em_proyxing_queue_create

f241353

goto failed

c3732d5

Nicer struct initialization syntax

be9db82

Split out task_queue_grow

5b6294e

More "Not thread safe"

e5ed1ba

Split out task_queue_{de}init

8e17f91

Add test (excluding postMessage notification)

be3de8f

tlively force-pushed the new-proxying-api branch from ffc18e7 to be3de8f Compare December 14, 2021 05:58

tlively added 5 commits December 14, 2021 13:06

Test the recursion guard logic

f274a3c

Fix dangling tasks pointer

d147a5a

new postMessage notification

64c1d42

Test returner thread

4d5aa0e

test tasks_queue growth during processing

b421b3c

tlively changed the title ~~[WIP] New low-level proxying C API~~ New low-level proxying C API Dec 21, 2021

tlively marked this pull request as ready for review December 21, 2021 04:08

tlively requested review from sbc100, kripken and juj December 21, 2021 04:11

Fix test output

2a21b12

sbc100 approved these changes Dec 21, 2021

View reviewed changes

Address comments

82ea908

Put the emscripten_force_exit back

45f7b97

juj approved these changes Jan 4, 2022

View reviewed changes

tlively added 2 commits January 4, 2022 09:31

Expand comment and move EMSCRIPTEN_KEEPALIVE

3b9cbd8

rename to task_queue_is_empty

e9af913

tlively added 3 commits January 4, 2022 09:54

Expand recursion guard comment

fa0d198

Merge remote-tracking branch 'origin/main' into new-proxying-api

54ae773

Wait for returner to start up

e4b05eb

tlively added a commit that referenced this pull request Jan 4, 2022

Rewrite the old proxying API in terms of the new API

28be33e

Rewrite the old threading.h proxying API used for internal system call implementations in terms of the new proxying API introduced in #15737.

tlively mentioned this pull request Jan 4, 2022

Rewrite the old proxying API in terms of the new API #15880

Merged

Merge remote-tracking branch 'origin/main' into new-proxying-api

2e10028

tlively enabled auto-merge (squash) February 2, 2022 00:12

tlively disabled auto-merge February 2, 2022 01:48

tlively merged commit 7332f81 into main Feb 2, 2022

tlively deleted the new-proxying-api branch February 2, 2022 01:49

tlively mentioned this pull request Feb 4, 2022

Add synchronous dispatch to thread. #13315

Closed

tlively added a commit that referenced this pull request Mar 11, 2022

Rewrite the old proxying API in terms of the new API (#15880)

6650d80

Rewrite the old threading.h proxying API used for internal system call implementations in terms of the new proxying API introduced in #15737.

bastibl mentioned this pull request May 27, 2022

WebAssembly + WebUSB support libusb/libusb#1057

Closed

juj mentioned this pull request Jun 14, 2025

Pthread task queue deadlock race condition problem (with pthread_cancel()?) #24570

Open

New low-level proxying C API #15737

New low-level proxying C API #15737

Uh oh!

Conversation

tlively commented Dec 10, 2021

Uh oh!

tlively commented Dec 10, 2021

Uh oh!

sbc100 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tlively commented Dec 14, 2021

Uh oh!

tlively commented Dec 21, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sbc100 Dec 21, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

sbc100 Dec 21, 2021 •

edited

Loading