search

LiveUI / S3

S3 Client written in Swift
MIT License
93 stars 60 forks source link

`get` and `delete` cache issue? #19

Closed zdnk closed 5 years ago

zdnk commented 5 years ago

I am working on file management abstraction for Swift and Vapor 3 and I have found probably a bug in cache/header management.

On getting file and immediately after that trying to delete it or even after running it second time, I get on get or delete this:

errorResponse(NIOHTTP1.HTTPResponseStatus.notImplemented, S3.ErrorMessage(code: "NotImplemented", message: "A header you provided implies functionality that is not implemented", bucket: nil, endpoint: nil, header: Optional("If-Modified-Since"), requestId: Optional("69D5F968DFC541E3"), hostId: Optional("MpD4rDbYQxpw5gOzln5q3mOlTEhmW8kqVCx+5bk9+JOyx26bOv2IznWqwN554TEWwqMvQ+eu/5k=")))

You can see by pulling the project, fetching deps and running tests: https://github.com/zdnk/vapor-filesystem/tree/4f6187b1c8d7306e78c1dbb5d74d752acf0c97db Commit: 4f6187b1c8d7306e78c1dbb5d74d752acf0c97db

I am using there the latest RC of version 3 as you can see in Package files.

zdnk commented 5 years ago

The issue is indeed URLSession cache. I tried calling URLCache.shared.removeAllCachedResponses() in the tests and it fixed issue. I think this should be raised with Vapor tho, because there is no cache control as far as I know.

rafiki270 commented 5 years ago

I already got one of them through here https://github.com/vapor/vapor/pulls?q=is%3Apr+is%3Aclosed+author%3Arafiki270

Unfortunately the one which alows for custom configuration didn’t make it through

zdnk commented 5 years ago

@rafiki270 is there anything we can do to speed this up? it is half a year already. Or simply use workaround? (directly using URLSession) instead of FoundationClient

rafiki270 commented 5 years ago

tbh, if we use URLSession (and move one of the crypto methods out) we might be able to get rid of the vapor dependency all together ...

zdnk commented 5 years ago

Perfect! I totally vote with all my 20 fingers for this. Suddenly you can have framework-agnostic server-side Swift S3 package. With more changes (using blocks instead of Futures) you can even have iOS package and do NIO futures as extension only with separate package :)

rafiki270 commented 5 years ago

:) I would still like to keep the NIO Futures for compatibility

zdnk commented 5 years ago

Well if you release just this fix, then yes, if you make a breaking change, then it can make sense, but I understand, the goal is server-side (Vapor) for now. So are you ok If I open PR for removing the Vapor.Client dependency and use URLSession directly?

rafiki270 commented 5 years ago

yeah man

zdnk commented 5 years ago

Ok cool. Ill keep dependency on Vapor.Container for now for backward compatibility.

zdnk commented 5 years ago

PR created, hopefully, you guys will be able to review it, merge it and release a new version soon.

iGranDav commented 5 years ago

Since the last PR is merged, you should probably close this issue?

rafiki270 commented 5 years ago

yes, thanks @iGranDav :)