During testing the GC will run after every test. #173

Warfields · 2022-11-16T01:35:56Z

Automatically run garbage collection after each EW test to catch kj::Own/IoOwn issues.

src/workerd/io/worker-entrypoint.c++

kentonv · 2022-11-16T18:32:21Z

src/workerd/jsg/setup.h

@@ -77,6 +77,9 @@ class IsolateBase {
  // Immediately cancels JavaScript execution in this isolate, causing an uncatchable exception to
  // be thrown. Safe to call across threads, without holding the lock.

+  void requestGcForTesting() const;


I'd put this method on jsg::Lock rather than IsolateBase.

Will do. Just curious, what is your reasoning on putting it on the jsg::Lock instead of IsolateBase

It makes your code simpler as you don't have to do IsolateBase::from().

In general jsg::Lock is the place for all general JavaScript runtime functionality that isn't connected to a specific object.

I'd put this method on jsg::Lock rather than IsolateBase.

hmm.. really? I'd prefer it not be too visible on an API that is more generally used. Too easy for someone to see and abuse/misuse at some point.

I'm quite happy hiding this away on something somewhere it's less visible.

Too easy for someone to see and abuse/misuse at some point.

The name makes clear this is only for testing, and the implementation will not work if it's not in test mode. People "abusing" this isn't a real risk, but if it were, adding superficial road blocks doesn't seem like the right way to stop them. It just makes the code ugly.

There is a fairly large warning on the definition in the header file. But I could see how it might be an issue, someone could use it intending that it run outside of tests and all of our testing would pass. But, the second it rolls out it would cause fatal crashes out in the wild. I do think that the name would steer people away from it, but it might be a good idea to rename to requestGcForTestingOnly so that it sticks out like a sore thumb in future code reviews if someone uses it.

You make a really good point, actually... the problem with throwing an exception when called outside of tests is that it'll pass tests and fail in prod.

This might argue for changing the logic so it merely logs an error and returns (without actually running a GC) if executed in production. At least then we'll see it in Sentry.

Another possibility is to use #ifdef KJ_DEBUG around the whole thing and all call sites, so that the method isn't even defined in release builds. Then if the release build compiles at all we know it's not using this.

I think that the log and return w/o executing is the cleanest way of doing this. That way we don't have to maintain an implicit contract to call #ifdef KJ_DEBUG all over the place. Having Sentry ping the team is actually a really good way to ensure that the function is used correctly even if someone accidentally slips it into a PR that makes it past review.

Changed it to logging an error and returning before running the GC.

Warfields · 2022-11-16T18:56:11Z

Yeah, that makes sense.

…

On Wed, Nov 16, 2022 at 11:55 AM Kenton Varda ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In src/workerd/jsg/setup.h <#173 (comment)>: > @@ -77,6 +77,9 @@ class IsolateBase { // Immediately cancels JavaScript execution in this isolate, causing an uncatchable exception to // be thrown. Safe to call across threads, without holding the lock. + void requestGcForTesting() const; It makes your code simpler as you don't have to do IsolateBase::from(). In general jsg::Lock is the place for all general JavaScript runtime functionality that isn't connected to a specific object. — Reply to this email directly, view it on GitHub <#173 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AH3SDLG5FDEIKMYTL6PZQZLWIUUZNANCNFSM6AAAAAASBTRHII> . You are receiving this because you authored the thread.Message ID: ***@***.***>

Warfields · 2022-11-16T21:14:01Z

Added the GC to all entry points now that the jsg::Lock call has solidified

src/workerd/jsg/setup.c++

src/workerd/jsg/jsg.h

src/workerd/jsg/jsg.c++

src/workerd/jsg/jsg.h

src/workerd/io/worker-entrypoint.c++

Warfields · 2022-11-17T23:59:13Z

Merge conflict fixed

jasnell

LGTM once @kentonv is happy with it.

kentonv · 2022-11-18T18:20:55Z

src/workerd/jsg/jsg.c++

+    KJ_LOG(ERROR, "Test GC used while not in a test");
+    return;
+  }
+  IsolateBase::from(v8Isolate).requestGcForTesting();


You can just do v8Isolate->RequestGarbageCollectionForTesting(...) here and delete the method from IsolateBase.

kentonv · 2022-11-18T18:21:59Z

src/workerd/io/worker-entrypoint.c++

@@ -5,9 +5,11 @@
 #include "worker-entrypoint.h"
 #include <capnp/message.h>
 #include <workerd/jsg/jsg.h>
+#include <workerd/jsg/setup.h>


I think this include is no longer used?

kentonv · 2022-11-18T18:22:33Z

Just a couple trivial nitpicks left. I'm about to head to the airport but James can merge when those are fixed.

Warfields requested review from jasnell, dom96, vickykont and kentonv November 16, 2022 01:37

Warfields force-pushed the swarfield/testingGc branch from e62e351 to bd81acd Compare November 16, 2022 01:41

Warfields commented Nov 16, 2022

View reviewed changes

src/workerd/io/worker-entrypoint.c++ Outdated Show resolved Hide resolved

Warfields force-pushed the swarfield/testingGc branch from bd81acd to a964472 Compare November 16, 2022 18:17

kentonv reviewed Nov 16, 2022

View reviewed changes

src/workerd/io/worker-entrypoint.c++ Outdated Show resolved Hide resolved

Warfields force-pushed the swarfield/testingGc branch from a964472 to ba04fb9 Compare November 16, 2022 18:23

kentonv reviewed Nov 16, 2022

View reviewed changes

Warfields force-pushed the swarfield/testingGc branch 2 times, most recently from d97de85 to 35743b4 Compare November 16, 2022 19:51

jasnell reviewed Nov 16, 2022

View reviewed changes

src/workerd/jsg/setup.c++ Outdated Show resolved Hide resolved

jasnell reviewed Nov 16, 2022

View reviewed changes

src/workerd/jsg/jsg.h Outdated Show resolved Hide resolved

Warfields force-pushed the swarfield/testingGc branch from 35743b4 to a44299b Compare November 16, 2022 23:34

vickykont reviewed Nov 17, 2022

View reviewed changes

src/workerd/jsg/jsg.c++ Outdated Show resolved Hide resolved

vickykont reviewed Nov 17, 2022

View reviewed changes

src/workerd/jsg/jsg.h Outdated Show resolved Hide resolved

Warfields force-pushed the swarfield/testingGc branch from a44299b to ce462d4 Compare November 17, 2022 22:48

kentonv reviewed Nov 17, 2022

View reviewed changes

src/workerd/io/worker-entrypoint.c++ Outdated Show resolved Hide resolved

Warfields force-pushed the swarfield/testingGc branch 3 times, most recently from 6837370 to af8f5d6 Compare November 17, 2022 23:58

Warfields force-pushed the swarfield/testingGc branch from af8f5d6 to 5e62894 Compare November 18, 2022 02:08

Warfields requested a review from vickykont November 18, 2022 03:07

Warfields force-pushed the swarfield/testingGc branch from 5e62894 to fd28068 Compare November 18, 2022 17:33

Warfields requested review from kentonv and jasnell November 18, 2022 17:33

Warfields force-pushed the swarfield/testingGc branch from fd28068 to bd0e4db Compare November 18, 2022 17:50

jasnell approved these changes Nov 18, 2022

View reviewed changes

kentonv reviewed Nov 18, 2022

View reviewed changes

Warfields force-pushed the swarfield/testingGc branch 2 times, most recently from abced7c to 1d520cd Compare November 18, 2022 20:31

During testing the GC will run after every test.

db6784b

Warfields force-pushed the swarfield/testingGc branch from 1d520cd to db6784b Compare November 18, 2022 20:46

Warfields merged commit 1f8a561 into main Nov 18, 2022

Warfields deleted the swarfield/testingGc branch November 18, 2022 22:05

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

During testing the GC will run after every test. #173

During testing the GC will run after every test. #173

Warfields commented Nov 16, 2022

kentonv Nov 16, 2022

Warfields Nov 16, 2022

kentonv Nov 16, 2022

jasnell Nov 16, 2022

kentonv Nov 17, 2022

Warfields Nov 17, 2022

kentonv Nov 18, 2022

Warfields Nov 18, 2022

Warfields Nov 18, 2022

Warfields commented Nov 16, 2022 via email

Warfields commented Nov 16, 2022

Warfields commented Nov 17, 2022

jasnell left a comment

kentonv Nov 18, 2022

Warfields Nov 18, 2022

kentonv Nov 18, 2022

Warfields Nov 18, 2022

Warfields Nov 18, 2022

kentonv commented Nov 18, 2022

During testing the GC will run after every test. #173

During testing the GC will run after every test. #173

Conversation

Warfields commented Nov 16, 2022

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

Warfields commented Nov 16, 2022 via email

Warfields commented Nov 16, 2022

Warfields commented Nov 17, 2022

jasnell left a 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

kentonv commented Nov 18, 2022