Skip to content

Conversation

@ormsbee
Copy link
Contributor

@ormsbee ormsbee commented Jan 11, 2026

A more modest version of #37842

Having a global modulestore reference is a holdover from the early days,
when the XML modulestore would be extremely expensive to initialize.
That is no longer the case, and we should eliminate the global here to
reduce the chances of multi-threading bugs that might mutate the global.

Unfortunately, a lot of code currently assumes that calls to the
modulestore() function are essentially free, instead of the 1-2 ms it
really is. There are cases where this may be called thousands of times
deep in loops somewhere while doing course content traversal, so it's
risky to eliminate the caching behavior altogether. The thought here is
that we'll tie to the RequestCache, which should at least not be shared
across users/threads.

@ormsbee ormsbee changed the title convert modulestore global to request cache WIP: refactor: convert global modulestore to request-cached Jan 11, 2026
@ormsbee ormsbee force-pushed the convert-modulestore-global-to-request-cache branch 3 times, most recently from 49ec474 to 0fb612a Compare January 11, 2026 17:41
@ormsbee ormsbee requested a review from a team as a code owner January 11, 2026 20:51
@ormsbee
Copy link
Contributor Author

ormsbee commented Jan 13, 2026

@robrap and @timmc-edx flagged that 2U has seen operational issues related to having too many MongoDB connections:

@robrap: In the last year we had a production incident where a runaway scaling event brought down production because of too many connections to Mongo. If I remember correctly, it was around 80-100 LMS webapp instances. I don't know how useful this is, but still worth being aware of.
tmccormack

@timmc-edx: And notably, I think this was only with maybe 3-4x as many webapp instances as we normally have.

@ormsbee ormsbee force-pushed the convert-modulestore-global-to-request-cache branch 5 times, most recently from 1e21654 to 095a87b Compare January 15, 2026 15:11
Having a global modulestore reference is a holdover from the early days,
when the XML modulestore would be extremely expensive to initialize.
That is no longer the case, and we should eliminate the global here to
reduce the chances of multi-threading bugs that might mutate the global.

Unfortunately, a lot of code currently assumes that calls to the
modulestore() function are essentially free, instead of the 1-2 ms it
is without caching. There are cases where this may be called thousands of
times deep in loops somewhere while doing course content traversal, so
it's risky to eliminate the caching behavior altogether. The thought here
is that we'll tie to the RequestCache, which should at least not be shared
across users/threads.

Since new instances of the modulestore will not have cached entries for
a particular course, it means that some query count tests have to be
adjusted upward.
@ormsbee ormsbee force-pushed the convert-modulestore-global-to-request-cache branch from 095a87b to d7825b2 Compare January 15, 2026 15:12
@ormsbee ormsbee changed the title WIP: refactor: convert global modulestore to request-cached refactor: convert global modulestore to request-cached Jan 15, 2026
@ormsbee ormsbee closed this Jan 15, 2026
@ormsbee ormsbee deleted the convert-modulestore-global-to-request-cache branch January 15, 2026 16:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant