Bug 694206

Summary:	Swallowed exceptions in client upload API
Product:	[Retired] Pulp	Reporter:	Jay Dobies <jason.dobies>
Component:	z_other	Assignee:	Jeff Ortel <jortel>
Status:	CLOSED CURRENTRELEASE	QA Contact:	Preethi Thomas <pthomas>
Severity:	unspecified	Docs Contact:
Priority:	unspecified
Version:	unspecified	Keywords:	Triaged
Target Milestone:	---
Target Release:	Sprint 22
Hardware:	Unspecified
OS:	Unspecified
Whiteboard:
Fixed In Version:		Doc Type:	Bug Fix
Doc Text:		Story Points:	---
Clone Of:		Environment:
Last Closed:	2012-02-24 20:16:21 UTC	Type:	---
Regression:	---	Mount Type:	---
Documentation:	---	CRM:
Verified Versions:		Category:	---
oVirt Team:	---	RHEL 7.3 requirements from Atomic Host:
Cloudforms Team:	---	Target Upstream Version:

Description Jay Dobies 2011-04-06 17:53:07 UTC

In Momento.read and .delete, if an exception occurs it isn't even logged. In read especially, I can't imagine we want to proceed with whatever is using it if it was unable to be read, but that's what the code will do.

While you're in there, it's spelled "memento" not "momento". And on line 45:

  path = path = os.path.join(root, str(checksum))

The second path assignment is just sloppy.

Comment 1 Jeff Ortel 2011-04-06 21:22:16 UTC

In this case it's normal for the memento to not exist so the code is doing exactly what it is intended to do.  However, not catching a specific exception could hide a syntax error so I updated to catch (and discard) a specific exception.  Also, added a comment so that it is clearer that this is intended.  Testing for os.path.exists() or other such tests still leaves the door open for race conditions and further complicates error handling here.  The only error condition this could potentially hide is the user intentionally changing permissions on ~/.pulp/upload or the memento itself.  The result would be that the memento is ignored and an interrupted upload would just start from scratch.

As for Momento vs. Memento.  My bad.  Perhaps I was thinking of the "momento" (spanish).

The 2nd "sloppy" path assignment - that "typo" has been corrected.

Comment 2 Jeff Ortel 2011-04-11 14:08:08 UTC

build: 0.162

Comment 3 Jay Dobies 2011-04-13 19:23:01 UTC

Fixed in build 0.163.

Comment 4 Preethi Thomas 2011-09-02 14:45:51 UTC

Steps to verify

<jdob> - upload a file. a large one will make life easier on you since you'll have more time to break it
<jdob> - delete the temporary upload chunk
<jdob> so I think you can just delete ~/.pulp/upload
<jdob> then make sure the client log shows an exception

Comment 5 Preethi Thomas 2011-10-04 16:25:20 UTC

verified

[root@preethi ~]# rpm -q pulp
pulp-0.0.237-2.fc15.noarch


no exception in the log as per comment#1

Comment 6 Preethi Thomas 2012-02-24 20:16:21 UTC

Pulp v1.0 is released
Closed Current Release.

Comment 7 Preethi Thomas 2012-02-24 20:17:41 UTC

Pulp v1.0 is released.