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

Simple plugin system #1389

Merged
merged 25 commits into from
Apr 22, 2024
Merged

Conversation

thomas-zahner
Copy link
Member

@thomas-zahner thomas-zahner commented Mar 14, 2024

Intro

As discussed with you @mre this is a first fully functional version of a simple plugin system in lychee. The first approach of using a middleware system turned out to be more complex than necessary and it would not be possible to use Extism in that case, because there would be no way to pass a next function to Extism. Instead I came up with Chain and Chainable. Conceptually having a request chain and a response chain would be very similar to the middleware approach. Instead of a next function an element in a chain returns a ChainResult which can prematurely return a result in the chain.

The field request_chain is added to ClientBuilder and Client. This chain is traversed before making a request in check_website. check_file and check_mail are not affected. Remapping, exclusions and caching is not handled by the chain, because "they have no request".

Questions and concerns

Field in Client and ClientBuilder

Currently I have made request_chain of type Arc<Mutex<RequestChain>>. This is because reqwest::Request is not Clone but Client and ClientBuilder are Clone. Do you think it should be done this way? The alternative would probably be to update RequestChain to be Chain<OurOwnRequestType, Status>. When OurOwnRequestType implements Copy we probably would also facilitate the Extism plugins since copying data from/to the plugins is necessary.

Response chain

There is no response chain yet. Should I try to implement one in this PR?

Extism

This PR contains nothing related to Extism yet. I think it would be awesome if WASM plugins could be provided on the command line. So this would probably be lychee-bin loading the plugins and passing it to lychee-lib. Should I try add this in this PR? (edit: see #1418)

@thomas-zahner thomas-zahner requested a review from mre March 14, 2024 16:37
Copy link
Member

@mre mre left a comment

Choose a reason for hiding this comment

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

I like the design overall. Added some thoughts.

lychee-lib/src/chain/mod.rs Outdated Show resolved Hide resolved
lychee-lib/src/chain/mod.rs Outdated Show resolved Hide resolved
lychee-lib/src/client.rs Outdated Show resolved Hide resolved
lychee-lib/src/client.rs Outdated Show resolved Hide resolved
lychee-lib/src/client.rs Outdated Show resolved Hide resolved
lychee-lib/src/client.rs Outdated Show resolved Hide resolved
lychee-lib/src/quirks/mod.rs Outdated Show resolved Hide resolved
@mre
Copy link
Member

mre commented Mar 15, 2024

Currently I have made request_chain of type Arc<Mutex<RequestChain>>. This is because reqwest::Request is not Clone but Client and ClientBuilder are Clone. Do you think it should be done this way? The alternative would probably be to update RequestChain to be Chain<OurOwnRequestType, Status>. When OurOwnRequestType implements Copy we probably would also facilitate the Extism plugins since copying data from/to the plugins is necessary.

Yeah, that might make sense. I don't know. After all, reqwest::Request is an implementation detail.
From what I can see, the only field in reqwest::Request that isn't Clone is the Body, and that's because reqwest::Body also has a Stream variant (which is behind a feature flag)
https://docs.rs/reqwest/latest/src/reqwest/async_impl/body.rs.html#18-20
That means that, yeah, we can wrap reqwest::Request and make it Clone, but we'd lose the ability to use Stream, which I don't think we're planning to use.

There is no response chain yet. Should I try to implement one in this PR?

I added a comment for that. Maybe there's a way, but if it makes things too complicated, we can leave it out for now.

This PR contains nothing related to Extism yet. I think it would be awesome if WASM plugins could be provided on the command line. So this would probably be lychee-bin loading the plugins and passing it to lychee-lib. Should I try add this in this PR?

For now, we just need to make sure that Extism works with the design we have in mind. We can create a separate branch+PR, which is based on this branch and adds one plugin based on Extism as a proof-of-concept.

@mre
Copy link
Member

mre commented Mar 15, 2024

@HU90m, since this also affects our Architecture discussion, it would be great if you could also take a look. 😃

For some background, @thomas-zahner and me talked about a simple plugin-system offline and that it would be based on Extism. This first step is to work on the trait system, which will allow plugins in the future.

Copy link
Contributor

@HU90m HU90m left a comment

Choose a reason for hiding this comment

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

I really like this

What's the intended use of the ChainResult::Done? Is it so just that filters could be part of the request chain or will plugins be able to access IO?

@mre
Copy link
Member

mre commented Mar 20, 2024

@thomas-zahner can probably give a better answer here, but the idea is that at any point of the chain, a step can decide to stop the execution and return a final conclusion without passing the request on to the next step.
So filters would be part of the request chain, yes. For example the exclude filter could send a ChainResult::Done in case a URL got excluded.

Every plugin in the chain would be able to make network requests or access files, so yes, they will be able to access I/O, but I'm not sure what is the relation to ChainResult::Done in that case. (If you meant that Done would indicate that an I/O operation is done, then the answer is no, this would not be what it means. Instead, this is done with async/await, where an I/O operation will be .awaited and the runtime will take care of the rest.

@thomas-zahner
Copy link
Member Author

@HU90m Yes filters are a good example of a Chainable which return a ChainResult::Done. But it can be anything that wants to end the chain and make a status out of the request. I'm currently working on making the actual final HTTP request to be also a chainable which returns ChainResult::Done.

Copy link
Member Author

@thomas-zahner thomas-zahner left a comment

Choose a reason for hiding this comment

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

@mre The PR is ready for re-review. Basically this is a simple version of the plugin/chain system we could merge. We could still add a response chain and might make further use of Chainable for other parts. But I think this is enough as a first version.

On thing I can do before merging is to create a branch for the Extism prototype.

lychee-lib/src/checker.rs Outdated Show resolved Hide resolved
@thomas-zahner thomas-zahner marked this pull request as ready for review April 8, 2024 06:57
@mre
Copy link
Member

mre commented Apr 9, 2024

Looks fine.
We got two more action items:

@thomas-zahner
Copy link
Member Author

@mre See my latest commit for the try_clone().unwrap().

I've created an extism-prototype branch for a first simple but functional Extism-plugin system. Feel free to give any feedback.

Also make `Chainable` and  `ChainResult` public to support external plugins/handlers.
@mre
Copy link
Member

mre commented Apr 21, 2024

I think we want to make the Chainable trait and ChainResult public, so that library users can add their own handlers.
Created a PR on top of your branch for that and added a bit of documentation, since this will be a central part of lychee going forward.

thomas-zahner#3

You can review and merge into your branch if it looks fine. After that, let's get this shipped. 😅 Sorry for the long review delay.

@thomas-zahner thomas-zahner merged commit ddcca65 into lycheeverse:master Apr 22, 2024
7 checks passed
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.

4 participants