Simple plugin system

[thomas-zahner]

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)

[mre]

I like the design overall. Added some thoughts.

[mre]

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]

@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.

[HU90m]

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]

@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]

@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.

[thomas-zahner]

@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.

[mre]

Looks fine.
We got two more action items:

• Simple plugin system #1389 (comment)
• Check that the trait can be used in combination with extism.

[thomas-zahner]

@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.

[mre]

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.