-
Notifications
You must be signed in to change notification settings - Fork 71
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Inconsistent responses when PUTting media #1481
Comments
When doing a PUT normally (or my experience is) |
@whikloj that is my understanding too but I am seeing this when creating new media.They don't already exists. |
Then that's a bug 😞 The behaviour @whikloj described is the expected behaviour. The code checks to see if the entity already exists here: https://github.com/Islandora/islandora/blob/8.x-1.x/src/MediaSource/MediaSourceService.php#L255. So somehow that's getting fooled. Or its some sort of awful timing thing. What exactly are you doing to trigger it? |
It's possible that we are running into timing issues; where two items are trying to be created at the same time (media being uploaded and a derivative of another item). |
It's inconsistent (at least I can't reliably replicate it). I can collect a bit more info if want (that is, try to replicate it) and report back. Anything I should be looking for in particular? |
Running Workbench to create new nodes and associated media. I wanted to get the new media's URL, which is in the |
Is it reasonable to assume that if the controller generates a 204, the media already has a URL (i.e., a |
@mjordan Yes, exactly. You've already got what you seek. |
Still worried @seth-shaw-unlv may be right, though. Are you hammering it? Exactly how big of a batch are you making? |
Let me log at https://github.com/Islandora/islandora/blob/8.x-1.x/src/Controller/MediaSourceController.php#L176 and run workbench and see what's there. |
Hammering in the sense that I am creating one after the other, within parts of a second. But not hammering it the sense that I am only creating 2 nodes/media in one run, during debugging. |
@dannylamb you start your transaction at https://github.com/Islandora/islandora/blob/8.x-1.x/src/Controller/MediaSourceController.php#L163 , but do you need to explicitly close it? |
@mjordan IIRC the transaction autocommits when the variable goes out of scope. Let me go double check that 😅 |
According to Drupal docs, yes. They commit when the variable goes out of scope. But I don't see any harm in committing the transaction earlier sooner than later if we're running into timing issues. |
I'm referencing https://www.drupal.org/docs/8/api/database-api/transactions fwiw |
I'm not sure if it will matter, I just noticed that it wasn't being explicitly closed. I'll do some logging to see if I can come up with any more information. |
Ok, got something, I think: I've created about 100 media and didn't get any
Strange thing is, they don't correspond to |
For sure it's FITS. I get a bunch of those every time I debug it. If things aren't just so in the |
Still getting these errors frequently (maybe 20% of the time using a sample of 5 media doing some dev on Workbench) when PUTing media:
Would love to make REST more reilable.... anything we can do? I'm using a current Vagrant. |
As discussed in the 2020-10-07 tech call, I will comment out the transaction code to see if this particular error disappears. At least that will localize the problem and we can go from there. |
During the I8 call, I didn't want to take us on another tangent but if each POST is getting processed by an individual apache client thread, and a rollback can take a longer time (depending on what all had changed), but it got me wondering if the rollback names all being the same between these processes is a potential issue. Perhaps I do not entirely know what overhead it can be to use transactions in the first place (does it lock any records, does the system handle duplicate rollback names in parallel without confusing them, etc). While this would be a separate issue (and not likely behind the reason for an initial POST to fail), it seems to me that each thread would need its own database transaction (distinct name) for what changes happened during its processing until the commit can discard the transaction information. Initially my thought was that the first process that had to perform the rollback must have proceeded and causing the subsequent failed processes to NOT be able to rollback to their "savepoint_1" because the previous process disposed of it. |
@wgilling I don't know enough about how Drupal handles transactions to comment on your ideas. But here is the results of some testing I did:
|
@dannylamb up in #1481 (comment) you mentioned that "I don't see any harm in committing the transaction earlier sooner than later if we're running into timing issues." I'd be happy to test this. Would be good to track down this issue once and for all. Any suggestions? |
Decision at 2020-12-09 Tech call was that it's easier to change Workbench to use Drupal's file and media REST resources than to monkey with Islandora's MediaSourceController, which would be unnecessarily disruptive and non-trivial. Workbench issue is linked ^^. Thanks for the disuccsion everyone. @dannylamb I can close this if you want. |
i'm hitting this in our production environment without any use of the workbench module. |
@elizoller during a Migration? |
yes |
I am getting these errors with my migrations. I'm using isle-dc and workbench. It's triggered by both fits derivatives and thumbnail derivatives. I will be running more migrations next week and I will test if the suggestion in this issue fixes the problem. |
Was discussed in the Open Tech Call–a complex issue that could possibly be bumped up to a working group. Possible first steps to tackling: come up with a test that reproduces the issue. |
TAG met and decided to dedicate some of August's Open Meeting to a session where we figure out a way to fix this. |
Reading through the discussion at https://www.drupal.org/project/drupal/issues/1803886 I'd also note this comment related to the |
from @ruebot on the ICC call:
|
Creating media via HTTP
PUT
(docs) sometimes returns a201
and sometimes returns a204
. Does anybody know why this is? One practical implication of this is that when Drupal responds with a201
, alocation
header is returned:{'Date': 'Mon, 06 Apr 2020 15:38:20 GMT', 'Server': 'Apache/2.4.29 (Ubuntu)', 'X-Powered-By': 'PHP/7.2.29-1+ubuntu18.04.1+deb.sury.org+1', 'Cache-Control': 'must-revalidate, no-cache, private', 'Location': 'http://localhost:8000/media/193', 'X-UA-Compatible': 'IE=edge', 'Content-language': 'en', 'X-Content-Type-Options': 'nosniff', 'X-Frame-Options': 'SAMEORIGIN', 'Expires': 'Sun, 19 Nov 1978 05:00:00 GMT', 'Vary': '', 'X-Generator': 'Drupal 8 (https://www.drupal.org)', 'Content-Length': '0', 'Keep-Alive': 'timeout=5, max=100', 'Connection': 'Keep-Alive', 'Content-Type': 'text/html; charset=UTF-8'}
But when a
204
is returned, there is nolocation
:{'Date': 'Mon, 06 Apr 2020 15:38:24 GMT', 'Server': 'Apache/2.4.29 (Ubuntu)', 'X-Powered-By': 'PHP/7.2.29-1+ubuntu18.04.1+deb.sury.org+1', 'Cache-Control': 'must-revalidate, no-cache, private', 'X-UA-Compatible': 'IE=edge', 'Content-language': 'en', 'X-Content-Type-Options': 'nosniff', 'X-Frame-Options': 'SAMEORIGIN', 'Expires': 'Sun, 19 Nov 1978 05:00:00 GMT', 'Vary': '', 'X-Generator': 'Drupal 8 (https://www.drupal.org)', 'Keep-Alive': 'timeout=5, max=100', 'Connection': 'Keep-Alive'}
In both cases, the media appears to be created.
Some poking around in source code has led me to https://github.com/Islandora/islandora/blob/8.x-1.x/src/Controller/MediaSourceController.php#L143 as the source of this behavior but I am not clear on what triggers success or non-success. In both cases, the media appear to be created.
@dannylamb can you shed some light on this? Or any other @Islandora/8-x-committers ?
The text was updated successfully, but these errors were encountered: