search

nroi / flexo

a central pacman cache
MIT License
172 stars 10 forks source link

make DownloadOrder id an optional uuid non_cacheable_unique_id #84

Closed ppickfor closed 2 years ago

ppickfor commented 2 years ago

Hello,

I have discovered an issue when downloading the same package at the same time from two different clients when it is not yet cached.

The reason was that DownloadOrder id was unique for cacheable and uncacheable files. When the current order is looked up in the list of orders it is never found because each order was unique. This results is 2 simultaneous fetches of the file from the provider with different starting offsets in the destination.

This condition was never met let cached_size = if orders_in_progress.contains(&order) { debug!("order {:?} already in progress: nothing to do.", &order); return ScheduleOutcome::AlreadyInProgress;

This code was never executed ScheduleOutcome::AlreadyInProgress => { debug!("Job is already in progress"); let path = order.filepath(&properties); let complete_filesize: u64 = try_complete_filesize_from_path(&path)?; let content_length = complete_filesize - request.resume_from.unwrap_or(0); let file = File::open(&path)?; serve_from_growing_file(file, content_length, request.resume_from, client_stream)?; Ok(PayloadOrigin::RemoteMirror) }

I made the id optional and renamed it to non_cacheable_unique_id.

I did consider writing a test bit the test rig does not seem to make this straightforward.

This is my first rust pullrequest so any advise most welcome.

I have attached a debug log

Thanks

Peter flexo.log

nroi commented 2 years ago

That's a pretty serious bug, thank you for taking the time to look into it and fix it!

This is my first rust pullrequest so any advise most welcome.

Since you have explicitly asked for feedback, I'm going to comment on a few minor things, but don't bother changing anything. I'm going to merge this as-is and make a few minor changes later, because I want to have this bug fixed sooner rather than later. I'm going to name the branch with the improvements bugfix-concurrent-downloads, so just check out the branch if you're interested.

I did consider writing a test bit the test rig does not seem to make this straightforward. Don't worry, especially with bug fixes, I prefer it if people just go ahead and submit the PR instead of investing too much time understanding the test structure.