2

I have a website with basic crud operations that involve data and photos. I also need to extract the metadata for the photos that are being uploaded. My original implementation did not have var puts = new List<Action>(); so the following code

puts.Add(async () => await _s3Client.PutObjectAsync(BucketName, key, file.OpenReadStream(), file.Length, file.ContentType));

was originally written as

await _s3Client.PutObjectAsync(BucketName, key, file.OpenReadStream(), file.Length, file.ContentType);

Note: Im using AspNetBoilerplate so a few things arent shown in code.

  • SaveChanges() is called as long as the method is completed and no exception is thrown.
  • If an exception is thrown, the database does not get hit.
  • I am NOT handling exceptions thrown by ImageMetadataReader as this is handled via AspNetBoilerplate and using this as a way to know if all photos had metadata.

Consider having 3 files, 2 with meta, the last without.

With my previous implementation, the first 2 files would be placed in my bucket and the 3rd would fail and i would have no entries in my db table and an error would be shown to the user.

With the current implementation, i queue up the PutObjectAsync() into a List<Action> and once all the files are processed, i process the puts.

Everything seems to work, but I wonder if its a good design or if theres a better way.

    private async Task Create(CreateOrEditSnowEntriesDto input)
    {
        var snowEntries = ObjectMapper.Map<SnowEntries>(input);
        var entryId = await _snowEntriesRepository.InsertAndGetIdAsync(snowEntries);

        await _s3Client.EnsureBucketExists(BucketName);

        var puts = new List<Action>();
        foreach (var file in input.Files)
        {
            if (file.Length <= 0) continue;

            var fileName = Path.GetFileName(file.FileName);


            DateTime? date = null;
            GeoLocation location = null;

            var metadata = ImageMetadataReader.ReadMetadata(file.OpenReadStream());
            if (metadata.Any(a => a.GetType() == typeof(ExifSubIfdDirectory)))
            {
                var dateDirectory = metadata.OfType<ExifSubIfdDirectory>().FirstOrDefault();

                if (dateDirectory?.ContainsTag(ExifDirectoryBase.TagDateTimeOriginal) ?? false)
                    date = dateDirectory?.GetDateTime(ExifDirectoryBase.TagDateTimeOriginal);
            }

            if (metadata.Any(a => a.GetType() == typeof(GpsDirectory)))
            {
                var gpsDirectory = metadata.OfType<GpsDirectory>().FirstOrDefault();
                location = gpsDirectory?.GetGeoLocation();
            }

            // EntryId/File.ext
            var key = $"{entryId}/{fileName}";
            puts.Add(async () => await _s3Client.PutObjectAsync(BucketName, key, file.OpenReadStream(), file.Length, file.ContentType));

            await _snowEntries_PhotosRepository.InsertAsync(new SnowEntries_Photos()
            {
                EntryId = entryId,
                FileName = fileName,
                Key = key,
                DateTaken = date,
                LocationTaken = location?.CreateCoordinates()
            });
        }

        foreach (var put in puts)
        {
            put.Invoke();
        }
    }
  • Instead of queuing the task, couldn't you just queue the File object instead? Also consider using something like input.Files.Where(file => file.Length > 0) so that you won't need the continue; statement – Ben Cottrell Feb 2 at 9:58
0

There are a few issues that I see with the current approach:

1. Data consistency/user error notification.

The original Action probably had one very important property - you saved all the data to all the required storages (some relation DB through EF and some S3 storage) and told the user that everything is done, or something went wrong and you sent error page back.

Now such consistency is no longer present in the action.

If something fails while saving the metadata to S3, then

  • User won't be notified.
  • You will no longer possess the required metadata corresponding to the records in the relational DB.

Well, it is possible that for the system as a whole

  • user experience will not be affected by the absent metadata
  • and a few non-user facing missing records won't matter much

But it is still a point worth considering.

2. Offloading background jobs with (async)Action.Invoke()

Action.Invoke() in the current snippet just basically offloads the loading [no pun intended] of the metadata to some threads on the thread pool.

What is wrong with that? Well:

  1. Some of the issues mentioned in https://haacked.com/archive/2011/10/16/the-dangers-of-implementing-recurring-background-tasks-in-asp-net.aspx/ still apply - basically such jobs may be running on the borrowed time and nothing guarantees that they will even be allowed to try to execute in some cases.
  2. Those are the same threads by regular requests, so you may end up stealing (though for a relatively small time) a lot of your own threads.
  3. And the most important - it will usually work, but that is not the intended way to offload background jobs in ASP.NET.

As it has been already mentioned in another answer, it is much better to use a standard way to offload the job - some library/service like Hangfire or background tasks with hosted services

P.S. 1: While S3 client seems to be thread-safe, it still looks relatively dangerous to just offload any kind of a job (for example, DbContext async methods are not thread-safe).

P.S. 2: And also there is a question of lifetime of the request data (input file streams file.OpenReadStream()) - I am not fully sure that if the job starts after the request has been already processed there won't be any issues with the stream (it may end up already disposed/unavailable).

-1

I think you should look at Hangfire https://www.hangfire.io/. You could queue your task, and then trigger another one once everything is completed. You'd have your Job stored in the database and could re-run it, view the results etc.

Another option is BackgroundService in Asp.Net Core which is specifically designed for what you need: https://docs.microsoft.com/en-us/aspnet/core/fundamentals/host/hosted-services?view=aspnetcore-2.2

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.