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

refactor: worker is no longer a resource #3290

Merged

Conversation

@bartlomieju
Copy link
Contributor

bartlomieju commented Nov 7, 2019

Exploring yet another approach to handling Worker.

This PR changes make it that Worker is no longer a resource but rather CLI internal structure. When creating child workers they are placed inside ThreadSafeState.workers (previously only shared future of Worker was placed there).

With this changes it becomes quite easy to scope resources per worker (no sharing of resources between workers, resource tables are created per worker). Also it opens a way to implement changes described in #3116.

There's still some cleanup to do, but I wanted to get feedback about this approach.

CC @ry @piscisaureus

bartlomieju added 5 commits Nov 7, 2019
fix
@ry

This comment has been minimized.

Copy link
Collaborator

ry commented Nov 7, 2019

So in this change a worker has an id but it's not a resource. There is a separate special table for workers to be stored? What are the problems caused by having a worker be a resource?

@bartlomieju

This comment has been minimized.

Copy link
Contributor Author

bartlomieju commented Nov 8, 2019

So in this change a worker has an id but it's not a resource. There is a separate special table for workers to be stored? What are the problems caused by having a worker be a resource?

That's correct. The reason is that worker was "split" into 3 parts before; internal channels (held inside state), external channels (that was the resource) and "shared future" of worker (again held in state in workers field). It led to situation where worker finished it's job but was never actually closed because resource was not removed nor was this shared future removed from workers table. This PR makes workers self-contained and spawning sub workers are only ever owned/referenced by their' parent worker.

Because workers are special - they are basically new instances of V8 isolate (with some additional Deno infra) I believe it makes sense to treat them in a bit special way and not expose anyhow to interaction by user except Worker API (eg. doing close(worker_rid) crashes the process, I think you did it in one of the talks btw).

Besides when we take into account #3116 then it really makes sense to have self-contained workers that are not anyhow tied to resources (but rather worker "own" the resources in per-worker resource tables)

@ry ry requested a review from piscisaureus Nov 8, 2019
@ry

This comment has been minimized.

Copy link
Collaborator

ry commented Nov 8, 2019

My initial thought is that it sounds reasonable.

Summoning @piscisaureus for a second opinion.

@bartlomieju

This comment has been minimized.

Copy link
Contributor Author

bartlomieju commented Nov 8, 2019

Although I felt this is a right way forward I'm not entirely sure now. When you look at #3282 there's mention of SharedWorker.

The SharedWorker interface represents a specific kind of worker that can be accessed from several browsing contexts, such as several windows, iframes or even workers.

If we're interested in supporting SharedWorkers this PR would make it kinda impossible to do that, but it's overall an upgrade to current situation. We might tackle it later when we'll be tackling resources shared between workers. WDYT?

@piscisaureus

This comment has been minimized.

Copy link
Collaborator

piscisaureus commented Nov 8, 2019

I have no objections per se to this patch. It looks quite clean by itself, and at this time I think a per-worker resource table is the way to go.
That said, I can't fully oversee the consequences for SharedWorker and shared resources that we might want to support in the future.

@piscisaureus piscisaureus merged commit 335e8bd into denoland:master Nov 9, 2019
10 checks passed
10 checks passed
test macOS-latest
Details
test_std macOS-latest
Details
test windows-2019
Details
test_std windows-2019
Details
test ubuntu-16.04
Details
test_debug ubuntu-16.04
Details
test_std ubuntu-16.04
Details
bench ubuntu-16.04
Details
lint ubuntu-16.04
Details
license/cla Contributor License Agreement is signed.
Details
@bartlomieju bartlomieju deleted the bartlomieju:refactor-worker_resource_take_2 branch Nov 9, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
3 participants
You can’t perform that action at this time.