Problem/Motivation

FileUploadResource uses a lock to prevent duplicate file name uploads. The lock is never released when an exception is thrown during file validation.

Steps to reproduce

1) Send a file that fails validation, for example because of file size.
2) Send the same file name with correct file size.
3) You get a 503 locked exception although the upload should work.

Proposed resolution

Always release the lock with a finally {} block in FileUploadResource. The same thing was done in GraphQL module in https://github.com/drupal-graphql/graphql/pull/1118

Remaining tasks

2) How could we test this?

CommentFileSizeAuthor
#4 3184974-test-only.patch1.43 KBraman.b

Issue fork drupal-3184974

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks

Comments

klausi created an issue. See original summary.

klausi opened merge request !72

klausi’s picture

klausi
he/him||they/them
Primary language German
Location 🇦🇹 Vienna
commented
Issue summary: View changes
Status: Active » Needs review
Issue tags: +Needs tests

Opened merge request with the proposed try/finally block.

raman.b’s picture

raman.b commented
StatusFileSize
new 3184974-test-only.patch1.43 KB

Expanded test coverage for \Drupal\Tests\rest\Functional\FileUploadResourceTestBase::testFileUploadLargerFileSize() to emulate steps to reproduce from IS. But couldn't get a failing test case

1) Send a file that fails validation, for example because of file size.
2) Send the same file name with correct file size.
3) You get a 503 locked exception although the upload should work.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented

Woah, nice catch!

dww’s picture

dww
we/he/they
commented
Component: file.module » rest.module

Alas, I pointed out exactly the same at #2940383-59: [META] Unify file upload logic of REST and JSON:API.3. I should have opened a bug report about it in rest.module when I noticed that. Sorry!

dww’s picture

dww
we/he/they
commented
Component: rest.module » file.module
Status: Needs review » Needs work
Issue tags: +Bug Smash Initiative

I guess this is a REST plugin provided by file.module, so restoring the component. ;)

I'm sorta reluctant to +1 this, even though it's a bug, since we're trying to unify all this code at #2940383: [META] Unify file upload logic of REST and JSON:API and this is a big diff to conflict with that. But given the track record on how long #2940383 is taking, the pragmatic thing is probably to fix this as a stand-alone bug and then we'll have to re-roll #2940383 again.

Regardless, this still needs a (failing) test that shows the underlying bug. I think we'd need to be explicitly checking that the lock is still around. That's not going to show up with the kinds of assertions being added in #4...

Version: 9.2.x-dev » 9.3.x-dev

Drupal 9.2.0-alpha1 will be released the week of May 3, 2021, which means new developments and disruptive changes should now be targeted for the 9.3.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.3.x-dev » 9.4.x-dev

Drupal 9.3.0-rc1 was released on November 26, 2021, which means new developments and disruptive changes should now be targeted for the 9.4.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.4.x-dev » 9.5.x-dev

Drupal 9.4.0-alpha1 was released on May 6, 2022, which means new developments and disruptive changes should now be targeted for the 9.5.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.5.x-dev » 10.1.x-dev

Drupal 9.5.0-beta2 and Drupal 10.0.0-beta2 were released on September 29, 2022, which means new developments and disruptive changes should now be targeted for the 10.1.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 10.1.x-dev » 11.x-dev

Drupal core is moving towards using a “main” branch. As an interim step, a new 11.x branch has been opened, as Drupal.org infrastructure cannot currently fully support a branch named main. New developments and disruptive changes should now be targeted for the 11.x branch, which currently accepts only minor-version allowed changes. For more information, see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

kim.pepper’s picture

kim.pepper
Primary language English
Location 🏄‍♂️🇦🇺Sydney, Australia
commented
Status: Needs work » Postponed (maintainer needs more info)

I suspect this is not a bug since we always release locks at the end of a request with a shutdown function.

See https://git.drupalcode.org/project/drupal/blob/e0b4b7ef6e6997d9446e60486...

kim.pepper’s picture

kim.pepper
Primary language English
Location 🏄‍♂️🇦🇺Sydney, Australia
commented
Status: Postponed (maintainer needs more info) » Closed (outdated)

Since this issue was created, we moved implementation to the common FileUploadHandler which now handles locking in a finally block.
https://git.drupalcode.org/project/drupal/-/blob/11.x/core/modules/file/...

I think we can close this issue. Please re-open if you think otherwise.

nicxvan closed merge request !72