elazarl / go-bindata-assetfs

Serves embedded files from `jteeuwen/go-bindata` with `net/http`
BSD 2-Clause "Simplified" License
870 stars 116 forks source link

Add support for AssetInfo #23

Closed keegancsmith closed 8 years ago

keegancsmith commented 9 years ago

Currently only uses ModTime from AssetInfo.

keegancsmith commented 9 years ago

I think this will currently break the API of anyone relying on NewAsset* or who is using positional arguments on AssetFS. I'm not sure what the best approach to move forward is, but this is a nice change to have.

Motivation: Currently the modtime returned is the time of the binary starting. If you have two instances behind a reverse-proxy they will return different modtimes. This causes cache control headers to frequently invalidate.

dmitshur commented 9 years ago

I could be wrong, but it seems unlikely that there would be code relying on NewAsset* since the whole point of this library is to expose the output of go-bindata as a http.FileSystem interface. In fact, I'm not sure why that func is exported, since it seems to be a part of internal API of go-bindata-assetfs.

It would break if using unkeyed composite literals, but not if using field-keyed syntax.

It's not up to me to make API decisions here, but it seems like a reasonable and favorable change. go-bindata already captures and exposes modtimes, and it's a part of http.FileSystem interface too. My vfsgen fork already has this feature, it preserves the modtimes of input.

The implementation details LGTM.

keegancsmith commented 8 years ago

@elazarl ping

elazarl commented 8 years ago

@keegancsmith thanks, apologize for the delay. Sounds like a good idea.

alde commented 8 years ago

Just FYI:

This causes a runtime error if you don't specify AssetInfo when creating the struct, and later try to access the data.

See klarna/eremetic#44

ryanuber commented 8 years ago

Just adding a note here for anyone who comes looking for this answer: if you start seeing panics after this change, it could be that you need to re-generate and update the output of go-bindata-assetfs. In my case, we were checking the resulting Go code into the repo after generation, and saw these panics during testing:

panic: runtime error: invalid memory address or nil pointer dereference [recovered]
    panic: runtime error: invalid memory address or nil pointer dereference
[signal 0xb code=0x1 addr=0x0 pc=0x7e904e]
goroutine 6499 [running]:
testing.tRunner.func1(0xc82150c5a0)
    /usr/local/go/src/testing/testing.go:450 +0x171
github.com/elazarl/go-bindata-assetfs.(*AssetFS).Open(0xc821e0bef0, 0xc82164e2b0, 0xf, 0x0, 0x0, 0x0, 0x0)
    /home/travis/gopath/src/github.com/elazarl/go-bindata-assetfs/assetfs.go:148 +0x22e
net/http.serveFile(0x2b9f5b923f80, 0xc821f980a0, 0xc8217a9880, 0x2b9f5b954b88, 0xc821e0bef0, 0xdfd190, 0x1, 0xdfd201)
    /usr/local/go/src/net/http/fs.go:394 +0x5b1
...
...
elazarl commented 8 years ago

Hmm... I was assuming that people who update their go-bindata-assetfs dir, would also rerun the binary.

Do you think it's better to revert this change? I'm not sure.

On Fri, Dec 25, 2015 at 5:04 AM, Ryan Uber notifications@github.com wrote:

Just adding a note here for anyone who comes looking for this answer: if you start seeing panics after this change, it could be that you need to re-generate and update the output of go-bindata-assetfs. In my case, we were checking the resulting Go code into the repo after generation, and saw these panics during testing:

panic: runtime error: invalid memory address or nil pointer dereference [recovered] panic: runtime error: invalid memory address or nil pointer dereference [signal 0xb code=0x1 addr=0x0 pc=0x7e904e] goroutine 6499 [running]: testing.tRunner.func1(0xc82150c5a0) /usr/local/go/src/testing/testing.go:450 +0x171github.com/elazarl/go-bindata-assetfs.(*AssetFS).Open(0xc821e0bef0, 0xc82164e2b0, 0xf, 0x0, 0x0, 0x0, 0x0) /home/travis/gopath/src/github.com/elazarl/go-bindata-assetfs/assetfs.go:148 +0x22e net/http.serveFile(0x2b9f5b923f80, 0xc821f980a0, 0xc8217a9880, 0x2b9f5b954b88, 0xc821e0bef0, 0xdfd190, 0x1, 0xdfd201) /usr/local/go/src/net/http/fs.go:394 +0x5b1 ... ...

— Reply to this email directly or view it on GitHub https://github.com/elazarl/go-bindata-assetfs/pull/23#issuecomment-167184570 .