Use async for filesystem hits

[RealOrangeOne]

Except Directory, as there's no async-compatible glob implementation

Description

Use tokio::fs when interacting with the filesystem.

Motivation and Context

This frees the event loop up for doing other things when waiting for the filesystem.

How Has This Been Tested?

The compiler doesn't complain, and very little else has changed.

I'd try the unit tests, but those fail on master, let alone this branch.

Changelog Entry

Types of Changes

• [ ] Bug fix (non-breaking change which fixes an issue)
• [ ] New feature (non-breaking change which adds functionality)
• [ ] Breaking change (fix or feature that would cause existing functionality to change)
• [ ] Documentation (no code change)
• [ ] Refactor (refactoring production code)
• [ ] Other

Checklist:

• [ ] My code follows the code style of this project.
• [ ] I have updated the documentation accordingly.
• [ ] I have formatted the code with rustfmt.
• [ ] I checked the lints with clippy.
• [ ] I have added tests to cover my changes.
• [ ] All new and existing tests passed.

[RealOrangeOne]

I've not done any benchmarks from this, but I'd expect most of the benefit to come in throughput as opposed to raw latency. Adding more yields adds more overhead to a given request, but allows the server to handle other requests at the same time. I doubt rustypaste is used in any particularly high-throughput deployments, but can't hurt to be that little more efficient. It also means more concurrent requests can be handled by fewer workers.

[orhun]

Sounds good to me. Clippy is failing, can you check?

I think this PR will be ready to merge when the CI is green!

[tessus]

I have noticed that the tests fail - in-code and fixtures.

[tessus]

what's up with this PR? The tests fail and it should be merged so that I can work on the fix for files with the same name.

Can we get this moving again? First the tests have to pass.

[RealOrangeOne]

Even the master test suite fails locally for me, so I don't trust my local environment (or there's something else weird going on). I think I found the root cause of the main outstanding failures, which I've just fixed. Hopefully, the tests now pass properly

[tessus]

Even the master test suite fails locally for me,

You are correct. I've opened an issue #253 to track this issue. However, there are only 2 tests failing in git master. But I recall there were quite a lot failing here.

[RealOrangeOne]

There are only 3 failing here, all looking like they may be related. I'll rebase and see if that resolves anything.

[tessus]

There is still one test case failing. Not sure why that is when it's ok in git master: https://github.com/orhun/rustypaste/actions/runs/8167143469/job/22327067458#step:5:517

P.S.: I can try running your branch locally and check out the tests. But I am way past my bed time. I'll have to do that tomorrow.

[RealOrangeOne]

Possibly a race in the IO, or something else behaving weirdly. I'll try and look this evening.

[tessus]

I ran the tests in your branch several times locally and there was never an error. All tests passed. @orhun any chance you could try the tests in this branch locally?

I also looked at the test and it is a bit confusing. It converts the body of the response (which is the link that is returned from rustypaste) into bytes. Why the conversion? Anyway, the same link is supposed to be returned for a file that has the same contents. I ran a rustypaste test instance (master branch) and uploaded the same file twice (with duplicate_files = false) and I got 2 different links returned. That should not have happened. But when I run the tests all is fine. How is this possible? This makes no sense.

Maybe orhun has a better crasp on this, since he wrote the code. I'm currently majorly confused.

[RealOrangeOne]

I can't reproduce the issues with cargo test (running that case specifically), but cargo tarpaulin does show an issue. I wonder if it's something to do with how tarpaulin runs, or something around concurrency. I'm not convinced it's related to this PR.

[tessus]

I looked at the code and I am even more confused now than I was before.

It seems that duplicate_files only works iff you do not send an expiry date and there is no default_expiry set in the config. I am not sure whether this was intended or maybe I am msreading the code. It also still doesn't explain why the tests fail on gh, but not locally.

[orhun]

For me all the tests pass locally. I'm not sure what's wrong with the CI :/

I also looked at the test and it is a bit confusing. It converts the body of the response (which is the link that is returned from rustypaste) into bytes. Why the conversion?

I'm guessing I had hard time converting body to a string (due to Pin and stuff) so gave up and just used bytes. But yeah, we should probably not do that.

It seems that duplicate_files only works if you do not send an expiry date and there is no default_expiry set in the config.

I think this was intended - since the other file has an expiry date it is "not the same" as the other file. It should be nice to document this behavior though.

[tessus]

I think this was intended - since the other file has an expiry date it is "not the same" as the other file. It should be nice to document this behavior though.

I don't really understand how to actually create a file that does not expire. Is it only possible, if no default is set? what about sending expiry as -1 or 0? Maybe something to add at one point? I never tried to create a file w/o expiration. Maybe it already works that way.

Even though the expiry date is not the same, the content of the file is the same. This is what tripped me up in my understanding.

But yeah, we should probably not do that.

other tests use string comparison, so it should be an easy fix. Still, it probably won't change the fact that it weirdly fails on gh.

[tessus]

other tests use string comparison, so it should be an easy fix. Still, it probably won't change the fact that it weirdly fails on gh.

ok, I see what the problem was and still is. I guess there's no way around it then. Sometimes I think that rust makes certain things overly complicated and not very intuitive.

[tessus]

It almost seems that on gh either one of the following is ignored:

• config.paste.duplicate_files = Some(false);
• that the default for default_expiry is not set

P.S.: The fixture also passes, so this is really weird.

[tessus]

I ran a few tests here #259 and it seems that tarpaulin has a side effect and makes that particular test fail.

I've opened an issue with tarpaulin: https://github.com/xd009642/tarpaulin/issues/1488

[RealOrangeOne]

So how can we progress this PR? It seems silly to hold it off because the coverage tool has a bug. Should we remove the coverage checker, merge this in, then create a separate PR to add it back?

[orhun]

I switched to cargo-llvm-cov (#260) until the issues with cargo-tarpaulin are fixed. Everything should be fine now 🤞🏼

[orhun]

I just realized an issue with this PR, hot-reloading the configuration file does not work for me. Just run cargo run and update config.toml - I expect to see "Configuration has been updated." and new values taking place but something is not working.

[RealOrangeOne]

Nice to see the tests passing!

Looks like it's not working because the watcher thread can't get an exclusive lock on the config - I think something is holding onto it for too long. I'll investigate.

[tessus]

Since my PR was merged, an .await haa to be added to the glob_match_file.

However, something is wrong after you force pushed. The changes that were merged from master by orhun are no longer in this PR.

[tessus]

@RealOrangeOne can you merge the changes in master back into this PR, change the one call to use .await, so that we can run the tests. Or you can give me push access to your branch and I do it.

P.S.: I think your force-push created a conflict.

[tessus]

The tests in paste.rs are missing the .await for the store_file call. What is strange though is that this was not caught by the lints.

P.S.: Why on earth would cargo clippy --tests --verbose -- -D warnings not catch this? How could the tests have been even compiled?

[RealOrangeOne]

Which ones? I've looked and all of them seem to call .await

[tessus]

Wait, I might have checked the wrong branch. Give me a sec. One test fails though.

[tessus]

ok, so I ran your branch locally and the tests pass. But on the ci the following happens:

thread 'paste::tests::test_paste_data' panicked at src/paste.rs:346:9:

[tessus]

I will run a few tests later this evening.

[tessus]

I ran a bunch of different modified tests and I am at a loss. I am stuck. No idea what is going on.

[tessus]

@orhun have you been able to figure out what the issue could be? it seems this is blocking a new release for a while now and the other PRs haven't been merged yet either.