search

grailbio / base

A collection of Go utility packages used by GRAIL's tools
Apache License 2.0
86 stars 24 forks source link

Use HeadObject for statRequest #16

Closed jcharum closed 4 years ago

jcharum commented 4 years ago

Use HeadObject instead of GetObject for handling statRequests. Besides being a better semantic match, it fixes #15.

jcharum commented 4 years ago

Does startGetObjectRequest still need to construct newInfo?

I don't think so, but I'm only 80% sure, as I have little experience with this code. My reasoning goes like this:

  1. startGetObjectRequest will only call newInfo if f.info == nil.
  2. startGetObjectRequest is only called by handleRead.
  3. handleRead is only called to handle a readRequest.
  4. readRequest is only issued in (*s3Reader).Read.
  5. The only way to get an *s3Reader is by (*s3File).Reader. It only does this if f.mode == readonly.
  6. The only way to get a readonly *s3File is by (*s3Impl).Open.
  7. (*s3Impl).Open will only return successfully if the handleStat sends a non-error response, via runRequest and statRequest.
  8. handleStat only sends a non-error response after populating f.info.
  9. Therefore, f.info != nil in startGetObjectRequest, and it will not call newInfo.

I'm not really sure how to thoroughly test removal of this code right now, so I don't want to add it to this PR. The change in this PR seems much less risky.

Also, I looked through the history, and I think the f.info == nil case became obsolete in 3cf2bd3a402b890b1e04641627c2044fb9fe986e, which introduced the statRequest on (*s3Impl).Open.