tahoe-lafs/tahoe-lafs
tahoe-lafs
/
tahoe-lafs
2
0
Fork 0
Code Issues Pull Requests 23 Packages Projects Releases 1 Wiki Activity

Improved HTTP response length handling #1204

Merged
itamarst merged 17 commits from 3907-http-response-lengths into master 2022-07-22 16:37:17 +00:00
Conversation 2 Commits 17 Files Changed 8 +187 -42
itamarst commented 2022-07-20 16:12:23 +00:00 (Migrated from github.com)
Copy Link

Fixes https://tahoe-lafs.org/trac/tahoe-lafs/ticket/3907

There's a few error handling codepaths that aren't handled, like "mutable changed out from under you" or "there's a bug!". Can try to figure something out if reviewer feels it's useful.

Fixes https://tahoe-lafs.org/trac/tahoe-lafs/ticket/3907 There's a few error handling codepaths that aren't handled, like "mutable changed out from under you" or "there's a bug!". Can try to figure something out if reviewer feels it's useful.
itamarst commented 2022-07-20 18:47:44 +00:00 (Migrated from github.com)
Copy Link

BOOOOOO

2022-07-20T18:45:16.4509660Z coveralls.exception.CoverallsException: Could not submit coverage: 504 Server Error: Gateway Time-out for url: https://coveralls.io/api/v1/jobs
BOOOOOO ``` 2022-07-20T18:45:16.4509660Z coveralls.exception.CoverallsException: Could not submit coverage: 504 Server Error: Gateway Time-out for url: https://coveralls.io/api/v1/jobs ```
coveralls commented 2022-07-20 20:31:35 +00:00 (Migrated from github.com)
Copy Link

Coverage Status

Coverage decreased (-0.04%) to 94.701% when pulling 94e0568653 on 3907-http-response-lengths into 69f90e6336 on master.

[![Coverage Status](https://coveralls.io/builds/51090897/badge)](https://coveralls.io/builds/51090897) Coverage decreased (-0.04%) to 94.701% when pulling **94e0568653a2fc49e33ba4be8c1f86b82aa48737 on 3907-http-response-lengths** into **69f90e6336c58a22143c5928099597c85a73c252 on master**.
exarkun (Migrated from github.com) approved these changes 2022-07-21 20:52:52 +00:00
exarkun (Migrated from github.com) left a comment
Copy Link

Thanks. Looks good. Some minor comments inline. Please address those and then merge.

Thanks. Looks good. Some minor comments inline. Please address those and then merge.
docs/proposed/http-storage-node-protocol.rst
exarkun (Migrated from github.com) commented 2022-07-21 12:29:30 +00:00
Copy Link
If the response reads beyond the end of the data, the response may be shorter than the requested range.
```suggestion If the response reads beyond the end of the data, the response may be shorter than the requested range. ```
docs/proposed/http-storage-node-protocol.rst
exarkun (Migrated from github.com) commented 2022-07-21 12:30:09 +00:00
Copy Link
 If the response reads beyond the end of the data, the response may be shorter than the requested range.
```suggestion If the response reads beyond the end of the data, the response may be shorter than the requested range. ```
src/allmydata/storage/http_client.py
exarkun (Migrated from github.com) commented 2022-07-21 12:47:47 +00:00
Copy Link

Content-Range could be missing from the response header here, I think?

`Content-Range` could be missing from the response header here, I think?
exarkun (Migrated from github.com) commented 2022-07-21 20:18:36 +00:00
Copy Link
        # that possibility for now...
```suggestion # that possibility for now... ```
exarkun (Migrated from github.com) commented 2022-07-21 20:19:30 +00:00
Copy Link
            # conceivably could be a bug...
```suggestion # conceivably could be a bug... ```
src/allmydata/storage/immutable.py
@ -204,0 +207,4 @@
"""
Return the length of the data in the share, if we're reading.
"""
return self._length
exarkun (Migrated from github.com) commented 2022-07-21 20:49:14 +00:00
Copy Link

a docstring here would be nice

a docstring here would be nice
src/allmydata/storage/immutable.py
exarkun (Migrated from github.com) commented 2022-07-21 20:49:22 +00:00
Copy Link

and here

and here
src/allmydata/storage/mutable.py
@ -420,2 +421,4 @@
f.close()
return data_length
def check_write_enabler(self, write_enabler, si_s):
exarkun (Migrated from github.com) commented 2022-07-21 20:50:27 +00:00
Copy Link

and here

and here
src/allmydata/storage/server.py
exarkun (Migrated from github.com) commented 2022-07-21 20:51:39 +00:00
Copy Link

This could go directly to ShareFile I think?

This could go directly to `ShareFile` I think?
itamarst (Migrated from github.com) reviewed 2022-07-22 15:55:22 +00:00
src/allmydata/storage/immutable.py
itamarst (Migrated from github.com) commented 2022-07-22 15:55:21 +00:00
Copy Link

Ended up deleting this method, per later comment.

Ended up deleting this method, per later comment.
Sign in to join this conversation.
Reviewers
No reviewers
exarkun
Labels
Clear labels   
Benchmarking and Performance

   
HTTP Storage Protocol

   
Nevow Removal

   
Python 3 Porting

   
not-for-merge
No Label
Benchmarking and Performance
HTTP Storage Protocol
Nevow Removal
Python 3 Porting
not-for-merge
Milestone
Clear milestone
No items
No Milestone
Projects
Clear projects
No project
Assignees
Clear assignees
No Assignees
1 Participants
Notifications
Due Date
The due date is invalid or out of range. Please use the format 'yyyy-mm-dd'.

No due date set.

Dependencies

No dependencies set.

Reference: tahoe-lafs/tahoe-lafs#1204
No description provided.
Delete Branch "3907-http-response-lengths"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?