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

Support for large code #386

Closed
wants to merge 9 commits into from
Closed

Conversation

kadaan
Copy link
Contributor

@kadaan kadaan commented Oct 4, 2019

Fix for #360

@kadaan
Copy link
Contributor Author

kadaan commented Oct 4, 2019

Running code in the debugger is now a two step process:

  1. Post the compressed code to the api, which caches it and returns a uuid
  2. Store the uuid in a cookie the cookie and call /devtools/inspector.html

I've tried this locally and it works beautifully. Based on how I've configured express, I've been able to run with code that is up to 1mb compressed. This is handle basically any code that we could want to run.

A couple things to note.

  1. The uuid is random, so it should not be easy to guess an id to get the code.
  2. The code is compressed with lz-string on the client, stored compressed, and then decompressed for execution
  3. The code cache is an LRU which expires items after 5m and only keeps (this.config.maxConcurrentSessions + this.config.maxQueueLength) * 10 items.

@joelgriffith
Copy link
Collaborator

The only issue I can see with this, since it's not a two-step process, is that load-balancing across a fleet of instances might get trickier. Since it is a UI, I suppose it's possible to use cookies or something similar with nginx to do sticky load-balancing.

@kadaan
Copy link
Contributor Author

kadaan commented Oct 8, 2019

That is a valid concern. You have a couple calls at the moment that are similar, right? Feels like the same issue could be said about /workspace and /sessions.

I’m open to any other ideas you have to solve this issue.

@joelgriffith
Copy link
Collaborator

Good point! There are a few stateful things like sessions and workspace that have the same issue.

In that case I think having a two step process is our only option. Could we (instead of using cookies, which was the only way I was able to do this in one call) use a more idiomatic JSON API for sending in the code to debug? IE: POST /debugger { code: user-code }? I think your cache mechanism will work just fine (create the ID's and so forth), but we won't need as many libs since we can send bigger post requests.

Does that sound cool with you?

package.json Outdated Show resolved Hide resolved
src/code-cache.ts Outdated Show resolved Hide resolved
@kadaan
Copy link
Contributor Author

kadaan commented Oct 8, 2019

@joelgriffith QQ: If maxConcurrentSessions is set to 0 does the app fail to start?

@joelgriffith
Copy link
Collaborator

I don’t think so... I believe it just queues work but will never start it from what I recall of the libraries internals.

@joelgriffith
Copy link
Collaborator

I also don’t mind pushing this over the finish line since you’ve done a lot of work on this repo. Would also love to hear your thoughts on what could help future contributors

@kadaan
Copy link
Contributor Author

kadaan commented Oct 14, 2019

@joelgriffith I'm fine with a POST to /debugger. It would remove one endpoint, but the workflow would be basically the same.

@kadaan
Copy link
Contributor Author

kadaan commented Oct 14, 2019

@joelgriffith Just to be clear, you are thinking we should accept a POST to /debugger to cache the code (rather than the /cache I added) and then use it on the execute?

@kadaan
Copy link
Contributor Author

kadaan commented Jan 3, 2020

@joelgriffith Just to be clear, you are thinking we should accept a POST to /debugger to cache the code (rather than the /cache I added) and then use it on the execute?

@joelgriffith Am I correct about your thinking?

@joelgriffith
Copy link
Collaborator

Going to close this out as this will be fixed in a coming refactor

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.

2 participants