NSURL changes for special filepath for Storage Files#1215
Conversation
Description: This change adds the ability to create an NSURL with a Storage File and do all the things one would expect to be able to accomplish with it like reading and writing NSData and NSStreams. This is accomplished by making use of remnants of a "direct file" url and the Windows FutureAccessCache to retreive the file at a later point. EbrFile is refactored, minimized, and then extended to work with Storage Files. How verified: New functional test
|
@rajsesh-msft @memontic-ms @DHowett-MSFT @bviglietta please take a look #Closed |
| // now that a token has been acquired, form a file path URL similar to a file reference URL based on it. | ||
| return [self initFileURLWithPath:[NSString stringWithFormat:@"/.file/id=%@", [NSString _stringWithHSTRING:token.Get()]] | ||
| isDirectory:NO]; | ||
| } |
There was a problem hiding this comment.
I really don't like the use of special file: schema URLs for special behaviour... if there were some way to get this to interoperate with the existing URL protocol system, that'd be grand.
If not, I understand, and accept this as a cost... but it is clever and opaque. #Closed
There was a problem hiding this comment.
Yep I totally agree. It would be super grand to have a clear, coherent design here. Unfortunately the public apis for this force our hand a bit. The call pattern in specific that we are seeing for this scenario is:
UIImage *image = ... ;
if (!image && [imageURL isFileURL]) {
image = [UIImage imageWithContentsOfFile:[imageURL path]];
}
Which means the URL needs to return true for isFileURL and needs to interact withimageWithContentsOfFile when using just the raw path. The way to get this to interop easiest and most fully seemed to be to go ahead and use a real file URL with a custom path. Given that /.file/id=... URLs exist on the reference platform, this seemed sort of natural anyway but I definitely agree this isn't a clean design like I normally advocate for. :-(
| RETURN_NULL_IF_FAILED(accessList->get_MaximumItemsAllowed(&maxItems)); | ||
|
|
||
| // Documentation says 1000 but lets just make sure it was somthing valid. | ||
| RETURN_NULL_IF_FALSE(maxItems > 0); |
There was a problem hiding this comment.
ULL_IF_FALSE(maxItems > 0); [](start = 12, length = 27)
Is there a future world where they're unlimited and managed by the system? We may not want to break on those platforms. #Closed
| { | ||
| std::lock_guard<std::mutex> guard(s_lock); | ||
|
|
||
| // protect the count check and addint logic with a lock since we don't want to race to add the last guy. |
There was a problem hiding this comment.
addint [](start = 39, length = 6)
nit: spelling? #Closed
There was a problem hiding this comment.
yep commented myself on that. Looks like some comments are special and don't show up in all PR tools. Hooray innovation
| #include <COMIncludes.h> | ||
| #include "ErrorHandling.h" | ||
| #include <wrl/client.h> | ||
| #include <wrl\wrappers\corewrappers.h> |
|
|
||
|
|
||
| // | ||
| // UIApplicationTests |
There was a problem hiding this comment.
UIApplicationTests [](start = 3, length = 18)
NSURLStorageFileTests. Also below.
aballway
left a comment
There was a problem hiding this comment.
There are some places that are a bit sparse of comments but overall understandable
| @Status Interoperable | ||
| */ | ||
| - (instancetype)initWithStorageFile:(RTObject<WSIStorageFile>*)storageFile { | ||
| ComPtr<IInspectable> comPtr = [storageFile comObj]; |
There was a problem hiding this comment.
super nit: storageFile.comObj #Closed
| RETURN_NULL_IF_FAILED(accessList->get_Entries(&entries)); | ||
| RETURN_NULL_IF_FAILED(entries->get_Size(&count)); | ||
|
|
||
| if (count >= maxItems) { |
There was a problem hiding this comment.
should this remove an item when we have exactly maxItems? #Closed
| } | ||
|
|
||
| // now that a token has been acquired, form a file path URL similar to a file reference URL based on it. | ||
| return [self initFileURLWithPath:[NSString stringWithFormat:@"/.file/id=%@", [NSString _stringWithHSTRING:token.Get()]] |
There was a problem hiding this comment.
nit: use Strings::WideToNSString rather than directly using NSString internals #Closed
| // Special file type for cached storage files. | ||
| fileToAdd = EbrStorageFile::CreateInstance(file, mode, share, pmode); | ||
| if (fileToAdd) { | ||
| return EbrFile::AddFile(std::move(fileToAdd)); |
There was a problem hiding this comment.
nit: flip this if so this only has one return at the end #Closed
| } | ||
|
|
||
| int EbrFstat(int fd, struct stat* ret) { | ||
| return EbrFile::GetFile(fd)->Stat(ret); |
There was a problem hiding this comment.
do we want these to blow up when no file is found? #Closed
| int EbrStorageFile::Flush() { | ||
| std::lock_guard<std::recursive_mutex> lock(m_lock); | ||
|
|
||
| unsigned long long currentPosition{}; |
There was a problem hiding this comment.
super nit: add int to type #Closed
There was a problem hiding this comment.
There was a problem hiding this comment.
I don't like the implied int, but it's super not necessary hence super nit
| std::lock_guard<std::recursive_mutex> lock(m_lock); | ||
|
|
||
| unsigned long long currentPosition{}; | ||
| if (FAILED(m_randomAccessStream->get_Position(¤tPosition))) { |
| return -1; | ||
| } | ||
|
|
||
| if (!src) { |
There was a problem hiding this comment.
nit: make one if #Closed
| return -1; | ||
| } | ||
|
|
||
| UINT32 result; |
| TEST_CLASS_SETUP(NSURLStorageFileTestsSetup) { | ||
| return SUCCEEDED(FrameworkHelper::RunOnUIThread(&UIApplicationDefaultInitialize)); | ||
| } | ||
|
|
There was a problem hiding this comment.
add TEST_METHOD_CLEANUP so state doesn't leak into tests after this one #Closed
There was a problem hiding this comment.
| { | ||
| std::lock_guard<std::mutex> guard(s_lock); | ||
|
|
||
| // protect the count check and addint logic with a lock since we don't want to race to add the last guy. |
| return -1; | ||
| } | ||
|
|
||
| // TODO 1099: Add more properites that we can fill in stat. |
There was a problem hiding this comment.
perhaps remove TODO. could add things like last modified time / created time but that seems probably not needed and not all storage files may even have that. Maybe just wait and do it later if people want it? #Closed
There was a problem hiding this comment.
| } | ||
|
|
||
| int EbrStorageFile::Stat64i32(struct _stat64i32* ret) { | ||
| std::lock_guard<std::recursive_mutex> lock(m_lock); |
There was a problem hiding this comment.
consider a templated internal function to remove duplicated stat code.
| { | ||
| std::lock_guard<std::mutex> guard(s_lock); | ||
|
|
||
| // protect the count check and addint logic with a lock since we don't want to race to add the last guy. |
There was a problem hiding this comment.
yep commented myself on that. Looks like some comments are special and don't show up in all PR tools. Hooray innovation
| // now that a token has been acquired, form a file path URL similar to a file reference URL based on it. | ||
| return [self initFileURLWithPath:[NSString stringWithFormat:@"/.file/id=%@", [NSString _stringWithHSTRING:token.Get()]] | ||
| isDirectory:NO]; | ||
| } |
There was a problem hiding this comment.
Yep I totally agree. It would be super grand to have a clear, coherent design here. Unfortunately the public apis for this force our hand a bit. The call pattern in specific that we are seeing for this scenario is:
UIImage *image = ... ;
if (!image && [imageURL isFileURL]) {
image = [UIImage imageWithContentsOfFile:[imageURL path]];
}
Which means the URL needs to return true for isFileURL and needs to interact withimageWithContentsOfFile when using just the raw path. The way to get this to interop easiest and most fully seemed to be to go ahead and use a real file URL with a custom path. Given that /.file/id=... URLs exist on the reference platform, this seemed sort of natural anyway but I definitely agree this isn't a clean design like I normally advocate for. :-(
| [condition signal]; | ||
| }]; | ||
|
|
||
| [condition waitUntilDate:[NSDate dateWithTimeIntervalSinceNow:2]]; |
There was a problem hiding this comment.
2 [](start = 66, length = 1)
This might sporadically fail on the build server.
| - (BOOL)getPromisedItemResourceValue:(id _Nullable*)value forKey:(NSString*)key error:(NSError* _Nullable*)error STUB_METHOD; | ||
| - (NSDictionary*)promisedItemResourceValuesForKeys:(NSArray*)keys error:(NSError* _Nullable*)error STUB_METHOD; | ||
|
|
||
| // MSFT Additions |
There was a problem hiding this comment.
MSFT [](start = 3, length = 4)
WinOBJC
|
|
||
| // Ok now lets read it out!!! | ||
| NSData* readData = [NSData dataWithContentsOfURL:storageUrl]; | ||
| ASSERT_TRUE([readData isEqual:writeData]); |
There was a problem hiding this comment.
ASSERT_TRUE [](start = 4, length = 11)
Do we not have the OBJCEQ/OBJCNE macros in here? #Closed
| [condition signal]; | ||
| } | ||
|
|
||
| failure:^void(NSError* error) { |
There was a problem hiding this comment.
Did clang format this weird? #Closed
| #include "EbrDevRandomFile.h" | ||
| #include "EbrStorageFile.h" | ||
|
|
||
| static const wchar_t* TAG = L"PlatformSupport"; |
There was a problem hiding this comment.
PlatformSupport [](start = 30, length = 15)
This should be EbrFile now, presumably #Closed
| return EbrFile::GetFile(fd)->Stat64i32(ret); | ||
| } | ||
|
|
||
| intptr_t EbrGetOSFHandle(int fd) { |
There was a problem hiding this comment.
EbrGetOSFHandle [](start = 9, length = 15)
Does someone use this?
| mkdir(dirOut); | ||
| return true; | ||
| } | ||
| static std::regex drive("[a-zA-Z]:"); |
There was a problem hiding this comment.
regex [](start = 16, length = 5)
I know this isn't your code, but we're using a whole regex engine to detect a single letter and a colon?!
|
|
||
| std::shared_ptr<EbrFile> EbrIOFile::CreateInstance(const char* path, int mode, int share, int pmode) { | ||
| int fid{}; | ||
| auto result = _sopen_s(&fid, CPathMapper(path), mode, share, pmode); |
There was a problem hiding this comment.
auto [](start = 4, length = 4)
Only use auto when the return type cannot be easily expressed or is intuitively obvious from the righthand side.
| } | ||
|
|
||
| EbrIOFile::~EbrIOFile() { | ||
| if (m_fid != -1) { |
There was a problem hiding this comment.
m_fid != - [](start = 8, length = 10)
How do we get an EbrIOFile with a fid of -1?
| @@ -0,0 +1,351 @@ | |||
| //****************************************************************************** | |||
| // | |||
| // Copyright (c) Microsoft Corporation. All rights reserved. | |||
There was a problem hiding this comment.
Drop the corporation here. #Closed
| #include "EbrDevRandomFile.h" | ||
|
|
||
| std::shared_ptr<EbrFile> EbrDevRandomFile::CreateInstance(const char* path, int mode, int share, int pmode) { | ||
| if (strcmp(path, "/dev/urandom") == 0) { |
There was a problem hiding this comment.
strcmp [](start = 8, length = 6)
use strncmp #Closed
| #pragma once | ||
|
|
||
| #include <io.h> | ||
| #include <stdio.h> |
There was a problem hiding this comment.
include [](start = 1, length = 7)
nit: Import #Closed
| ComPtr<IRandomAccessStream>&& randomAccessStream, | ||
| bool shouldAppend, | ||
| bool shouldAllowWrite) : | ||
| m_storageFile(storageFile), |
There was a problem hiding this comment.
storageFile [](start = 26, length = 11)
Why pass-by-rvalue-reference with std::move on the outside (keeping external consumers aware of our internals) instead of pass-by-value with std::move on the inside? There does not appear to be consensus on this on the internet at large, but it looks like a lot of people lean towards the second.
|
Review status: 0 of 22 files reviewed at latest revision, 19 unresolved discussions. Frameworks/Foundation/NSURL.mm, line 38 at r1 (raw file):
|
| } | ||
|
|
||
| boolean success = false; | ||
| if (FAILED(AwaitResult(outStream, &IOutputStream::FlushAsync, &success))) { |
There was a problem hiding this comment.
AwaitResult [](start = 15, length = 11)
Will this not break due to the vtable thunk compiler bug?
| } | ||
|
|
||
| ComPtr<IBuffer> dataRead; | ||
| if (FAILED(AwaitProgressComplete(inStream, &IInputStream::ReadAsync, rawDataBuffer.Get(), buffer.size(), InputStreamOptions_None, &dataRead))) { |
There was a problem hiding this comment.
AwaitProgressComplete [](start = 15, length = 21)
As above? #Closed
There was a problem hiding this comment.
| } | ||
|
|
||
| int EbrFflush(int fd) { | ||
| return EbrFile::GetFile(fd)->Flush(); |
There was a problem hiding this comment.
GetFile [](start = 20, length = 7)
invalid pointer when we can't find the file?
Deal with grace or not? #Closed
| } | ||
|
|
||
| bool EbrRename(const char* path1, const char* path2) { | ||
| return rename(CPathMapper(path1), CPathMapper(path2)) == 0; |
There was a problem hiding this comment.
n rename(CPathMapper(path1), CPathMapper(path2)) == 0; [](start = 9, length = 54)
nit: Log errno if failure? #Closed
| /** | ||
| @Status Interoperable | ||
| */ | ||
| - (instancetype)initWithStorageFile:(RTObject<WSIStorageFile>*)storageFile { |
There was a problem hiding this comment.
initWithStorageFile [](start = 16, length = 19)
perhaps this should be initWithPathToStorageFile, since you can't really have a "URL" with a "storage file" #Closed
| char g_WritableFolder[2048] = "."; | ||
|
|
||
| void EbrSetWritableFolder(const char* folder) { | ||
| strcpy_s(g_WritableFolder, folder); |
There was a problem hiding this comment.
strcpy_s [](start = 3, length = 9)
This can fail, are we staying silent? log the error? #Closed
|
🕐 |
|
|
||
| #define FSROOT "." | ||
| #define mkdir _mkdir | ||
| char g_WritableFolder[2048] = "."; |
There was a problem hiding this comment.
2048 [](start = 22, length = 4)
where does this value come from?
MAX_PATH? #Closed
| return g_WritableFolder; | ||
| } | ||
|
|
||
| bool EbrGetRootMapping(const char* dirName, char* dirOut, uint32_t maxLen) { |
There was a problem hiding this comment.
dirOut [](start = 50, length = 6)
can this be invalid?
| return true; | ||
| } | ||
| if (_stricmp(dirName, "Documents") == 0) { | ||
| sprintf_s(dirOut, maxLen, "%s\\Documents", g_WritableFolder); |
There was a problem hiding this comment.
%s\Documents" [](start = 35, length = 14)
Put the directory names/path strings in an internal header as a const/define?
This would make future maintenance easier
|
|
||
| bool EbrGetRootMapping(const char* dirName, char* dirOut, uint32_t maxLen) { | ||
| if (dirName == NULL) { | ||
| strcpy_s(dirOut, maxLen, FSROOT); |
There was a problem hiding this comment.
strcpy_s [](start = 8, length = 8)
what is the copy fails? we end up returning true regardless.
| if (strcmp(ent.fileName, ".") == 0 || strcmp(ent.fileName, "..") == 0) | ||
| continue; | ||
|
|
||
| char fullPath[4096]; // max path? |
There was a problem hiding this comment.
4096 [](start = 34, length = 4)
MAX_PATH?
|
|
||
| if (result != 0) { | ||
| return nullptr; | ||
| } |
There was a problem hiding this comment.
if(_sopen_s(&fid, CPathMapper(path), mode, share, pmode) != 0 ) {
return nullptr;
} #Closed
| // Check to see if this path is a file id path. | ||
| static const std::string c_fileIdPreamble = "/.file/id="; | ||
|
|
||
| RETURN_NULL_IF_FALSE(0 == strncmp(path, c_fileIdPreamble.c_str() , c_fileIdPreamble.length())); |
There was a problem hiding this comment.
path [](start = 38, length = 4)
invalid path pointer? #Closed
|
Review status: 0 of 22 files reviewed at latest revision, 41 unresolved discussions. Frameworks/Starboard/EbrDevRandomFile.cpp, line 20 at r1 (raw file):
|
| RETURN_NULL_IF_FAILED(storageApplicationPermissionsStatics->get_FutureAccessList(&accessList)); | ||
|
|
||
| // See how close we are to the cap. | ||
| uint32_t maxItems = 0; |
There was a problem hiding this comment.
it would help to move this to a private function, so the init method isn't doing too many things..
|
|
| std::lock_guard<std::recursive_mutex> lock(m_lock); | ||
|
|
||
| unsigned long long currentPosition{}; | ||
| if (FAILED(m_randomAccessStream->get_Position(¤tPosition))) { |
There was a problem hiding this comment.
if (FAILED(m_randomAccessStream->get_Position(¤tPosition))) { [](start = 3, length = 68)
RETURN_RESULT_IF_FAILED
There was a problem hiding this comment.
I just realized this change is not in the repo. Still doesn't hurt to introduce it.
In reply to: 84979111 [](ancestors = 84979111)
| ComPtr<IOutputStream> outStream; | ||
| if (FAILED(m_randomAccessStream->GetOutputStreamAt(currentPosition, &outStream))) { | ||
| return -1; | ||
| } |
There was a problem hiding this comment.
as above, this and below can be covered via the MACRO
| unsigned char* rawBytes = nullptr; | ||
| if (FAILED(byteAccess->Buffer(&rawBytes))) { | ||
| return -1; | ||
| } |
There was a problem hiding this comment.
again, the macro introduction of RETURN_RESULT_IF_FAILED(.......,-1);
would help allot here.
|
Review status: 0 of 22 files reviewed at latest revision, 55 unresolved discussions. Frameworks/Starboard/EbrFile.cpp, line 3 at r1 (raw file):
drop corporation Comments from Reviewable |
| - (BOOL)getPromisedItemResourceValue:(id _Nullable*)value forKey:(NSString*)key error:(NSError* _Nullable*)error STUB_METHOD; | ||
| - (NSDictionary*)promisedItemResourceValuesForKeys:(NSArray*)keys error:(NSError* _Nullable*)error STUB_METHOD; | ||
|
|
||
| // MSFT Additions |
There was a problem hiding this comment.
MSFT [](start = 3, length = 4)
WINOBJC? #Closed
| // Lets write some data!!!! | ||
| unsigned char rawDataToWrite[] = { 'a', 'b', 'c' }; | ||
| NSData* writeData = [NSData dataWithBytesNoCopy:rawDataToWrite length:_countof(rawDataToWrite) freeWhenDone:NO]; | ||
| [writeData writeToURL:storageUrl options:0 error:nil]; |
There was a problem hiding this comment.
[writeData writeToURL:storageUrl options:0 error:nil]; [](start = 2, length = 56)
ASSERT_TRUE([writeData writeToURL:storageUrl options:0 error:nil]); #Closed
|
Frameworks/Starboard/EbrStorageFile.cpp, line 279 at r1 (raw file):
|
|
|
||
| // Now with appending! | ||
| writeData = [NSData dataWithBytesNoCopy:rawDataToWrite length:_countof(rawDataToWrite) freeWhenDone:NO]; | ||
| [writeData writeToURL:storageUrl options:NSDataWritingWithoutOverwriting error:nil]; |
There was a problem hiding this comment.
writeData writeToURL:storageUrl [](start = 5, length = 31)
check for success, as here and other writes #Closed
|
🕐 |
…validating my changes.
| ASSERT_FALSE_MSG(true, "FAILED: Waiting for download timed out!"); | ||
| } | ||
| [condition lock]; | ||
| ASSERT_TRUE_MSG( (downloadLocation || downloadResponse || downloadError) || |
There was a problem hiding this comment.
ASSERT_TRUE_MSG( (downloadLocation || downloadResponse || downloadError) || [](start = 3, length = 77)
some of these don't handle spurious wakeups well but its at least a lot better than before. NSCondition needs locking and if you signal before a wait it just goes into the ether so needed another piece of information to check against.
| SB_EXPORT int EbrFstat64i32(int fd, struct _stat64i32* ret); | ||
| SB_EXPORT int EbrRead(int fd, void* dest, size_t count); | ||
| SB_EXPORT int EbrWrite(int fd, const void* src, size_t count); | ||
| SB_EXPORT int EbrLseek(int fd, __int64 pos, int whence); |
There was a problem hiding this comment.
int [](start = 10, length = 3)
@DHowett-MSFT @msft-Jeyaram still working on this bit. Got very much sidetracked from test failures due to ridiculously wrong NSCondition use.
Description:
This change adds the ability to create an NSURL with a Storage File and do all the
things one would expect to be able to accomplish with it like reading and writing
NSData and NSStreams. This is accomplished by making use of remnants of a "direct file"
url and the Windows FutureAccessCache to retreive the file at a later point.
EbrFile is refactored, minimized, and then extended to work with Storage Files.
How verified:
New functional test
This change is