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

Implement HttpBlock #262

Merged
merged 2 commits into from
Feb 16, 2024
Merged

Implement HttpBlock #262

merged 2 commits into from
Feb 16, 2024

Conversation

vimpostor
Copy link
Contributor

This implements the HttpBlock as described in #236.

The WebAssembly implementation took quite some jumping through hoops to get right.
There are a lot of quirks with regard to threading in emscripten, that make the implementation a minefield of risking getting stuck in infinite loops, because the emscripten thread for various reasons is unable to make any progress.
Unfortunately we cannot use ASYNCIFY, because that is incompatible with exceptions support (that are already enabled).

There is also this fun emscripten regression, which means it's currently impossible to cancel asynchronously running fetch requests.

For the unit test the httplib dummy server obviously doesn't work with emscripten, so the --pre-js mechanism is used to implement another one with node http, that is automatically injected into the runtime.
Note that in order for the unit test to run, node needs to be able to find xhr2, so:

npm install xhr2

@vimpostor vimpostor linked an issue Feb 5, 2024 that may be closed by this pull request
@vimpostor vimpostor temporarily deployed to configure coverage February 5, 2024 12:13 — with GitHub Actions Inactive
@vimpostor vimpostor temporarily deployed to configure coverage February 5, 2024 12:13 — with GitHub Actions Inactive
@vimpostor vimpostor temporarily deployed to configure coverage February 5, 2024 12:13 — with GitHub Actions Inactive
@vimpostor vimpostor temporarily deployed to configure coverage February 5, 2024 12:13 — with GitHub Actions Inactive
@vimpostor vimpostor temporarily deployed to configure coverage February 5, 2024 12:18 — with GitHub Actions Inactive
@vimpostor vimpostor temporarily deployed to configure coverage February 5, 2024 12:18 — with GitHub Actions Inactive
@vimpostor vimpostor temporarily deployed to configure coverage February 5, 2024 12:18 — with GitHub Actions Inactive
@vimpostor vimpostor temporarily deployed to configure coverage February 5, 2024 12:18 — with GitHub Actions Inactive
@vimpostor vimpostor temporarily deployed to configure coverage February 5, 2024 12:45 — with GitHub Actions Inactive
@vimpostor vimpostor temporarily deployed to configure coverage February 5, 2024 12:45 — with GitHub Actions Inactive
@vimpostor vimpostor temporarily deployed to configure coverage February 5, 2024 16:42 — with GitHub Actions Inactive
@vimpostor vimpostor temporarily deployed to configure coverage February 5, 2024 16:42 — with GitHub Actions Inactive
@vimpostor vimpostor temporarily deployed to configure coverage February 5, 2024 16:42 — with GitHub Actions Inactive
@vimpostor vimpostor temporarily deployed to configure coverage February 5, 2024 16:42 — with GitHub Actions Inactive
@vimpostor vimpostor temporarily deployed to configure coverage February 5, 2024 17:13 — with GitHub Actions Inactive
@vimpostor vimpostor temporarily deployed to configure coverage February 5, 2024 17:13 — with GitHub Actions Inactive
@vimpostor vimpostor temporarily deployed to configure coverage February 5, 2024 17:13 — with GitHub Actions Inactive
@vimpostor vimpostor temporarily deployed to configure coverage February 5, 2024 17:13 — with GitHub Actions Inactive
@vimpostor
Copy link
Contributor Author

vimpostor commented Feb 15, 2024

@drslebedev I finished the tests rewrite using the scheduler. There were again some emscripten peculiarities (for some reason, the pre.js mechanism was not compatible with the setup of starting and stopping the server in each unit test), but everything should work now.

If the CI agrees, from my side this should be ready for review again.

Given that the graphs in your tests are nearly identical, encapsulating the graph creation and related setup into a function that returns either a Graph or Scheduler

I tried that, but except for the graph connections there is no boilerplate to be saved: The sink has to be accessible from within the test, so we cannot just encapsulate it in a createGraph() function. The HttpBlock instantiation differs in each unit test, so we can also not encapsulate that in such a function. The source also has to be accessible from the first unit test, so the only resolution would be to perfectly forward the HttpBlock instantiation parameters to the createGraph() function, then return a tuple of [source, httpBlock, sink] from that function which can then be destructured from the calling site. But I encountered some move issues with this and discovered that some template parameters cannot longer be automatically derived, resulting in more boilerplate than we save from wrapping the three graph connections in a function, so in the end I opted to just leave it as is (unless you know a more elegant solution).

@drslebedev
Copy link
Contributor

@vimpostor
Thank you for making those updates! The code looks good to me, and it can be merged once all the tests passed.

Could you please squash the commits into a single commit or possibly two? Given the presence of some work-in-progress commits, consolidating them will contribute to a cleaner commit history.

@vimpostor
Copy link
Contributor Author

vimpostor commented Feb 15, 2024

Could you please squash the commits into a single commit or possibly two?

Squashed it into two commits.

Copy link
Contributor

@drslebedev drslebedev left a comment

Choose a reason for hiding this comment

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

Thanks!
Can be merged.

This helps with writing unit tests.
This block can use both message ports as well as a public member
function as a trigger to make requests to a HTTP API (e.g. REST API) and
will return the response on its output ports.

Fixes #236
@RalphSteinhagen RalphSteinhagen merged commit 11d0767 into main Feb 16, 2024
1 check passed
@RalphSteinhagen RalphSteinhagen deleted the magnus/http-block branch February 16, 2024 15:03
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.

[5pt,9pt] HTTP block
4 participants