Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Replicator isn't including Authorization: header in requests, so it gets unnecessary 401s #1127

Closed
snej opened this issue Feb 18, 2016 · 3 comments
Closed

Replicator isn't including Authorization: header in requests, so it gets unnecessary 401s #1127

snej opened this issue Feb 18, 2016 · 3 comments
Assignees
@snej
Labels
bug f: Replication performance review
Milestone
1.2.1

Comments

@snej
Copy link
Contributor

snej commented Feb 18, 2016

The replicator isn't sending request credentials efficiently. When using HTTP Basic auth, it should only be getting one 401 response per session; after that it should be proactively including the Authorization: header in subsequent requests. But it isn't, so it's sending every request twice. This is obviously bad for performance.
@snej snej self-assigned this Feb 18, 2016
@zgramana zgramana added this to the 1.3 milestone Feb 19, 2016
@snej
Copy link
Contributor Author

snej commented Feb 20, 2016

This appears to be a bug in Apple's HTTP client implementation in CFNetwork. I've made a very small test case that reproduces it, and filed a bug report with Apple. (rdar://24753862).

We can work around this by directly adding an Authorization: header to every request. I've got a POC working but need to think about the details and edge cases some more before committing.

snej added a commit that referenced this issue Feb 21, 2016
@snej
Proactively set Authorization: header in HTTP requests
3e5eaef 
...when we have a CBLPasswordAuthorizer.
Also modified CBLRemoteRequest to never use the proposed credential,
since it might be for a different login to the same server.

Fixes #1127
snej added a commit that referenced this issue Feb 22, 2016
@snej
Proactively set Authorization: header in HTTP requests
fe7f777 
...when we have a CBLPasswordAuthorizer.
Also modified CBLRemoteRequest to never use the proposed credential,
since it might be for a different login to the same server.

This is equivalent to 3e5eaef on the master branch. Not directly
cherry-picked due to all the changes in CBLRemoteRequest, but I copied/
pasted the individual hunks.

Fixes #1127
@snej snej added the hotfix label Feb 22, 2016
@snej
Copy link
Contributor Author

snej commented Feb 22, 2016

Fixed on branch feature/nsurlsession, which will be merged into dev shortly.
I've also created a branch fix/1.2+basic_auth off of the 1.2 tag, containing just this fix.

@snej snej added review and removed in progress labels Feb 23, 2016
@snej
Copy link
Contributor Author

snej commented Feb 23, 2016

Merged into dev yesterday.

@snej snej closed this as completed Feb 23, 2016
@pasin pasin mentioned this issue Feb 25, 2016
Closed
@snej snej mentioned this issue Feb 28, 2016
Closed
@tleyden tleyden mentioned this issue Mar 2, 2016
Closed
@snej snej modified the milestones: 1.3, 1.2.1 Mar 3, 2016
snej added a commit that referenced this issue Mar 3, 2016
@snej
Proactively set Authorization: header in HTTP requests
1370497 
...when we have a CBLPasswordAuthorizer.
Also modified CBLRemoteRequest to never use the proposed credential,
since it might be for a different login to the same server.

This is equivalent to 3e5eaef on the master branch. Not directly
cherry-picked due to all the changes in CBLRemoteRequest, but I copied/
pasted the individual hunks.

Fixes #1127
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@snej snej

Labels
bug f: Replication performance review
Projects
None yet
Milestone
1.2.1
Development

No branches or pull requests
2 participants
@snej @zgramana