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

[WIP] feat: assetPath hook #3403

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

dmitry-stepanenko
Copy link
Contributor

No description provided.

tokio::runtime::Handle::current().block_on(async move {
self
.plugin_driver
.write()
Copy link
Collaborator

@h-a-n-a h-a-n-a Jun 6, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will be stuck if you would like to call this method from a JS Hook. As Rust called the JS hook in which you called this method is write, and if you call get_asset_path method, then, Rust calls the write of plugin_driver again, which leads to a deadlock. The only way to solve this is to call the plugin_driver as read.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it doesn't seem to help unfortunately. I've pushed a small commit so that get_asset_path method is async now and plugin_driver is called as read.

Are there any other ways to overcome this issue? As I can see we can't really invoke the hook from elsewhere or at another point of time, right?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are there any other ways to overcome this issue? As I can see we can't really invoke the hook from elsewhere or at another point of time, right?

If you want to acquire the mutable reference as what write does, you can only invoke the hook one at the time. But for hooks that only requires read, it does not have the limitation, which means you can invoke as much as you wish.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@h-a-n-a can you please point me what exactly should be changed in my implementation? I tried both read and write accesses and behavior is the same

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The deadlock problem has been resolved in #3455. You may continue to implement this hook. Thank you in advance!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks a lot @h-a-n-a !

@dmitry-stepanenko
Copy link
Contributor Author

@h-a-n-a this PR is pretty much ready, but there's an error thread '<unnamed>' panicked at 'there is no reactor running, must be called from the context of a Tokio 1.x runtime'. It's caused by this https://github.com/web-infra-dev/rspack/pull/3403/files#diff-5eb759d814281903aa0355174051f134df9444a46e9a1ce332cad27eed896215R1352

Any ideas how this can be avoided?

@h-a-n-a
Copy link
Collaborator

h-a-n-a commented Jun 15, 2023

@h-a-n-a this PR is pretty much ready, but there's an error thread '<unnamed>' panicked at 'there is no reactor running, must be called from the context of a Tokio 1.x runtime'. It's caused by this https://github.com/web-infra-dev/rspack/pull/3403/files#diff-5eb759d814281903aa0355174051f134df9444a46e9a1ce332cad27eed896215R1352

Any ideas how this can be avoided?

This is mainly because of the code that you're trying to run is not running in the context of Tokio runtime. You should create one in this case.

@dmitry-stepanenko
Copy link
Contributor Author

@h-a-n-a I've tried doing that but looks like I'm not able to come up with functional code. Would you be so kind to provide me a snippet how get_asset_path function should be changed in order to properly process the async code inside?

@h-a-n-a
Copy link
Collaborator

h-a-n-a commented Jun 20, 2023

@h-a-n-a I've tried doing that but looks like I'm not able to come up with functional code. Would you be so kind to provide me a snippet how get_asset_path function should be changed in order to properly process the async code inside?

Would you mind if I push some code to this PR?

@dmitry-stepanenko
Copy link
Contributor Author

dmitry-stepanenko commented Jun 20, 2023

Absolutely not!

@h-a-n-a
Copy link
Collaborator

h-a-n-a commented Jun 28, 2023

After I dig into this PR, I found out there's basically an issue about multi-thread and single-thread interoperation. Details will be added later.

@h-a-n-a
Copy link
Collaborator

h-a-n-a commented Jun 28, 2023

I initiated a discussion here: #3642 . Please allow me to set this PR to on -hold for now until we settle down to a solution.

@stale
Copy link

stale bot commented Aug 27, 2023

This pull request has been automatically marked as stale because it has not had recent activity. If this pull request is still relevant, please leave any comment (for example, "bump").

@stale stale bot added the stale label Aug 27, 2023
@dmitry-stepanenko
Copy link
Contributor Author

not stale

Copy link

stale bot commented Jan 6, 2024

This pull request has been automatically marked as stale because it has not had recent activity. If this pull request is still relevant, please leave any comment (for example, "bump").

@stale stale bot added the stale label Jan 6, 2024
@dmitry-stepanenko
Copy link
Contributor Author

not stale

@stale stale bot removed the stale label Jan 8, 2024
Copy link

stale bot commented Mar 8, 2024

This pull request has been automatically marked as stale because it has not had recent activity. If this pull request is still relevant, please leave any comment (for example, "bump").

@stale stale bot added the stale label Mar 8, 2024
@dmitry-stepanenko
Copy link
Contributor Author

not stale

@stale stale bot removed the stale label Mar 8, 2024
Copy link

stale bot commented May 7, 2024

This pull request has been automatically marked as stale because it has not had recent activity. If this pull request is still relevant, please leave any comment (for example, "bump").

@stale stale bot added the stale label May 7, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants