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.

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

BF: downloaders - lock (interprocess) initiation of the session or re-authentication #4308

Closed
wants to merge 4 commits into from

Conversation

yarikoptic
Copy link
Member

@yarikoptic yarikoptic commented Mar 16, 2020

This way parallel downloaders/special-remotes (e.g. as launched by git-annex) should not present multiple prompts for logins. Only one would handle it, and then the others would just follow (since credentials would then be known).

Closes #4284

yarikoptic added 4 commits Mar 15, 2020
…dition of locking

Looking at the moved variables use, there should be no change in behavior.
If there is -- a bug!
…authentication

That would prevent multiple "login" prompts whenever git-annex requests downloads
by our special remote in parallel.

It might incure slight slow down to initiate the connection(s) since only one
at a time would try, but IMHO it should not show any real impact since
typically time should be spent in transfers since ware to wrangle large
files primarily
@codecov
Copy link

@codecov codecov bot commented Mar 16, 2020

Codecov Report

Merging #4308 into maint will increase coverage by 0.01%.
The diff coverage is 90.62%.

Impacted file tree graph

@@            Coverage Diff             @@
##            maint    #4308      +/-   ##
==========================================
+ Coverage   89.65%   89.66%   +0.01%     
==========================================
  Files         275      275              
  Lines       36864    36872       +8     
==========================================
+ Hits        33051    33062      +11     
+ Misses       3813     3810       -3     
Impacted Files Coverage Δ
datalad/downloaders/base.py 76.72% <86.95%> (+1.54%) ⬆️
datalad/downloaders/tests/test_http.py 60.96% <100.00%> (+0.37%) ⬆️
datalad/support/locking.py 97.50% <100.00%> (-0.07%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4281360...14bae72. Read the comment docs.

kyleam
kyleam approved these changes Mar 16, 2020
Copy link
Contributor

@kyleam kyleam left a comment

Thanks. I didn't play around with anything on my end, but looks good to me on a read-through.

Subject: [PATCH 1/4] RF: BaseDownloader.access - moved code around to simplify
possible addition of locking

Looking at the moved variables use, there should be no change in behavior.
If there is -- a bug!

Looking at the --color-moved output, it does look to be mostly code movement and, from what I can tell, the end result should be the same.

Subject: [PATCH 3/4] BF: downloaders - lock upon establishing connection and
handling (re)authentication

That would prevent multiple "login" prompts whenever git-annex requests downloads
by our special remote in parallel.

It might incure slight slow down to initiate the connection(s) since only one
at a time would try, but IMHO it should not show any real impact since
typically time should be spent in transfers since ware to wrangle large
files primarily

Sounds reasonable.

Subject: [PATCH 4/4] BF(TST): we must allow avoid mocking dealing with a lock
file

For reviewers that aren't very familiar with the mocking in test_http (and didn't just spend time digging into it to try to understand a test failure), a small description of the problem would have been nice to see. From what I can gather, though, this looks fine to me.

op.join(
cfg.obtain('datalad.locations.cache'),
'locks',
'downloader-auth.lck'))
Copy link
Contributor

@kyleam kyleam Mar 16, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Based on the InterProcessLock documentation, I didn't see any guarantees that this directory is created if it doesn't exist. However, looking at the fasteners source code (and based on the passing tests), it does seem to be the case. Okay.

Copy link
Member Author

@yarikoptic yarikoptic Mar 16, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah -- I figured it "trial&error" way and didn't want to explicitly call mkdirs here since lock might not be needed (downloader not actually used) etc. So decided to stick on fasteners to provide it

@yarikoptic
Copy link
Member Author

@yarikoptic yarikoptic commented Mar 16, 2020

For reviewers that aren't very familiar with the mocking in test_http (and didn't just spend time digging into it to try to understand a test failure), a small description of the problem would have been nice to see. From what I can gather, though, this looks fine to me.

ok, I will adjust commit description locally, merge into maint and push to close this PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants