-
Notifications
You must be signed in to change notification settings - Fork 33
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
add option to retain fetchEvent handler #116
Closed
Closed
Changes from all commits
Commits
Show all changes
2 commits
Select commit
Hold shift + click to select a range
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -130,6 +130,10 @@ Setting `disableFeatures: ['random', 'stdio', 'clocks']` will disable all featur | |
|
||
Note that pure components **will not report errors and will instead trap**, so that this should only be enabled after very careful testing. | ||
|
||
The features that are not included by default are: | ||
* `'http'` - Support for sending and receiving HTTP requests, depends on `wasi:io` | ||
* `'fetch-event'` - Enables using `fetchEvent` to respond to `wasi:http/[email protected]#handle`. If the target world does note export `wasi:http/[email protected]`, this will be ignored. | ||
|
||
Note that features explicitly imported by the target world cannot be disabled - if you target a component to a world | ||
that imports `wasi:clocks`, then `disableFeatures: ['clocks']` will not be supported. | ||
|
||
|
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -124,7 +124,12 @@ pub struct Componentization { | |
pub resource_imports: Vec<(String, String, u32)>, | ||
} | ||
|
||
pub fn componentize_bindgen(resolve: &Resolve, id: WorldId, name: &str) -> Componentization { | ||
pub fn componentize_bindgen( | ||
resolve: &Resolve, | ||
id: WorldId, | ||
name: &str, | ||
retain_fetch_event: bool, | ||
) -> Componentization { | ||
let mut bindgen = JsBindgen { | ||
src: Source::default(), | ||
esm_bindgen: EsmBindgen::default(), | ||
|
@@ -342,6 +347,7 @@ pub fn componentize_bindgen(resolve: &Resolve, id: WorldId, name: &str) -> Compo | |
"$source_mod", | ||
&mut bindgen.local_names, | ||
name, | ||
retain_fetch_event, | ||
); | ||
|
||
let js_intrinsics = render_intrinsics(&mut bindgen.all_intrinsics, false, true); | ||
|
@@ -1001,6 +1007,7 @@ impl EsmBindgen { | |
imports_object: &str, | ||
_local_names: &mut LocalNames, | ||
source_name: &str, | ||
retain_fetch_event: bool, | ||
) { | ||
// TODO: bring back these validations of imports | ||
// including using the flattened bindings | ||
|
@@ -1043,6 +1050,10 @@ impl EsmBindgen { | |
"); | ||
} | ||
for (export_name, binding) in &self.exports { | ||
// If we are retaining the fetch event, then we should just let StarlingMonkey handle the event directly. | ||
if retain_fetch_event && export_name == "wasi:http/[email protected]" { | ||
continue; | ||
} | ||
match binding { | ||
Binding::Interface(bindings) => { | ||
uwrite!(output, "const "); | ||
|
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,6 +1,9 @@ | ||
use anyhow::{bail, Context, Result}; | ||
use bindgen::BindingItem; | ||
use std::path::{Path, PathBuf}; | ||
use std::{ | ||
collections::HashSet, | ||
path::{Path, PathBuf}, | ||
}; | ||
|
||
mod bindgen; | ||
mod splice; | ||
|
@@ -111,6 +114,7 @@ impl Guest for SpidermonkeyEmbeddingSplicerComponent { | |
wit_path: Option<String>, | ||
world_name: Option<String>, | ||
debug: bool, | ||
mut retain_fetch_event: bool, | ||
) -> Result<SpliceResult, String> { | ||
let source_name = source_name.unwrap_or("source.js".to_string()); | ||
|
||
|
@@ -131,7 +135,22 @@ impl Guest for SpidermonkeyEmbeddingSplicerComponent { | |
.map_err(|e| e.to_string())?; | ||
|
||
let mut wasm_bytes = wit_component::dummy_module(&resolve, world); | ||
let componentized = bindgen::componentize_bindgen(&resolve, world, &source_name); | ||
let componentized = | ||
bindgen::componentize_bindgen(&resolve, world, &source_name, retain_fetch_event); | ||
|
||
let target_world = &resolve.worlds[world]; | ||
let mut target_world_exports = HashSet::new(); | ||
|
||
for (key, _) in &target_world.exports { | ||
target_world_exports.insert(resolve.name_world_key(key)); | ||
} | ||
|
||
// Do not retain fetch event if the target world does not export the | ||
// incomingHandler Interface as that would make it incompatible with the | ||
// target world | ||
if !target_world_exports.contains("wasi:http/[email protected]") { | ||
retain_fetch_event = false; | ||
} | ||
|
||
// merge the engine world with the target world, retaining the engine producers | ||
let producers = if let Ok(( | ||
|
@@ -330,8 +349,8 @@ impl Guest for SpidermonkeyEmbeddingSplicerComponent { | |
// println!("{:?}", &imports); | ||
// println!("{:?}", &componentized.imports); | ||
// println!("{:?}", &exports); | ||
let mut wasm = | ||
splice::splice(engine, imports, exports, debug).map_err(|e| format!("{:?}", e))?; | ||
let mut wasm = splice::splice(engine, imports, exports, debug, retain_fetch_event) | ||
.map_err(|e| format!("{:?}", e))?; | ||
|
||
// add the world section to the spliced wasm | ||
wasm.push(section.id()); | ||
|
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -38,6 +38,7 @@ pub fn splice( | |
imports: Vec<(String, String, CoreFn, Option<i32>)>, | ||
exports: Vec<(String, CoreFn)>, | ||
debug: bool, | ||
retain_fetch_event: bool, | ||
) -> Result<Vec<u8>> { | ||
let mut config = walrus::ModuleConfig::new(); | ||
if debug { | ||
|
@@ -53,13 +54,16 @@ pub fn splice( | |
module.exports.delete(expt.id()); | ||
module.funcs.delete(run); | ||
} | ||
if let Ok(serve) = module | ||
.exports | ||
.get_func("wasi:http/[email protected]#handle") | ||
{ | ||
let expt = module.exports.get_exported_func(serve).unwrap(); | ||
module.exports.delete(expt.id()); | ||
module.funcs.delete(serve); | ||
|
||
if !retain_fetch_event { | ||
if let Ok(serve) = module | ||
.exports | ||
.get_func("wasi:http/[email protected]#handle") | ||
{ | ||
let expt = module.exports.get_exported_func(serve).unwrap(); | ||
module.exports.delete(expt.id()); | ||
module.funcs.delete(serve); | ||
} | ||
} | ||
|
||
// we reencode the WASI world component data, so strip it out from the | ||
|
@@ -79,7 +83,7 @@ pub fn splice( | |
synthesize_import_functions(&mut module, &imports)?; | ||
|
||
// create the exported functions as wrappers around the "cabi_call" function | ||
synthesize_export_functions(&mut module, &exports)?; | ||
synthesize_export_functions(&mut module, &exports, retain_fetch_event)?; | ||
|
||
Ok(module.emit_wasm()) | ||
} | ||
|
@@ -530,6 +534,7 @@ fn synthesize_import_functions( | |
fn synthesize_export_functions( | ||
module: &mut walrus::Module, | ||
exports: &Vec<(String, CoreFn)>, | ||
retain_fetch_event: bool, | ||
) -> Result<()> { | ||
let cabi_realloc = get_export_fid( | ||
module, | ||
|
@@ -561,6 +566,9 @@ fn synthesize_export_functions( | |
let arg_ptr = module.locals.add(ValType::I32); | ||
let ret_ptr = module.locals.add(ValType::I32); | ||
for (export_num, (expt_name, expt_sig)) in exports.iter().enumerate() { | ||
if retain_fetch_event && expt_name == "wasi:http/[email protected]#handle" { | ||
continue; | ||
} | ||
// Export function synthesis | ||
{ | ||
// add the function type | ||
|
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just like we automatically merge WASI imports into the target world, perhaps we can automatically merge the
wasi:http/[email protected]
export into the target world when this feature is used?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually isn't this the default behaviour anyway? Or do we still strip the export if it's not explicitly in the target world?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
With this PR we want to only optionally strip the export from the StarlingMonkey engine here based on the value of the feature. My intention with that statement is to make it such that, if the target world does not export the interface but the feature is enabled, we should probably still strip the export.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't it read "If the target world does export" though? Rather than "does not". Thinking about this further we probably want to make this an explicit error. That is, if you enable this feature and the target world already exports the incoming handler, then we should throw an error that you should either target the incoming handler export, or you should use the fetch event version of it, but not both.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think there is a disconnect in what we each mean by the target world. I am talking about the
wit
definition, not the guest content. As in, consider the following worldand the
fetch-event
is set, if we do not strip the export from the engine, it will end up in the output component but maybe that is fine?I also think that there is currently no way to check if a handler is attached to the fetch event but we can check if there is something that targets the incoming handler export so erroring on that would be good, I agree.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe I am misunderstanding here. To verify my understanding - the proposal above is to not retain
fetchEvent
incase the target world contains thewasi:http/incoming-handler
export and only have the feature take effect if it is not explicitly part of the target world.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, I see—I had missed that part of it. On that, I agree with you: I think this setting should be usable whether the targeted world includes
incoming-handler
or not.@guybedford doing what you proposed would make it substantially harder to use
FetchEvent
, because now anyone who wants to target an environment that supportswasi:http/proxy
has to edit the target world not to includeincoming-handler
if they want to useFetchEvent
. I think that's a requirement we really shouldn't put on people—and it'd be quite hard for us to satisfy using Spin as a development tool.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am happy to update this PR once there is consensus on this discussion.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Guy and I just discussed this further, and came to the realization that there's a cleaner way to do this: with #117, ComponentizeJS will only generate exports for symbols exported at the JS level, too. Based on that, we could simply say "if after treeshaking we still would export the symbol, then we'll do so. Otherwise, we'll leave it alone."
In effect, that'd mean that if the input StarlingMonkey already includes an export, and content doesn't specify that same export, ComponentizeJS will leave the original export alone.
@karthik2804, if that makes sense to you, then changing #117 accordingly should make it so that this issue isn't needed anymore.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I am roughly tracking it. Currently, #117 is purely for imports. I will take a stab at updating it and see if I understand things correctly and report back.