Fixed work with Shared Memory on Windows. #17273
Conversation
|
Hi @CoderAImen. And thanks for your contribution, what is your patch fixing precisely ? |
Corrects errors on request |
|
Alright then, will leave it to the appropriate folk. |
Corrections based on comments.
Syntax fixes
There was a problem hiding this comment.
Thank you for the PR! I see now that more needs to be done than I've suggested previously, but this looks a bit too much.
To summarize my understanding of the relevant resource management:
- each successful call to
CreateFileMapping()orOpenFileMapping()needs a matchingCloseHandle() - each successful call to
MapViewOfFile()needs a matchingUnmapViewOfFile()
| else if (TRUE == created) { | ||
| CloseHandle(shm->segment); | ||
| shm->segment = shm_handle; | ||
| shm->created = TRUE; | ||
| } |
There was a problem hiding this comment.
I don't understand what would be special about created. We either use OpenFileMapping() or CreateFileMapping() to get the shm_handle, and it seems to me that we want to close the shm->segment in neither case.
| @@ -51,6 +51,8 @@ typedef struct { | |||
| typedef struct { | |||
| void *addr; | |||
| HANDLE segment; | |||
| BOOL created; | |||
| int shm_nattch; | |||
There was a problem hiding this comment.
Okay, I see that we need to track the number of attachments for each process/thread, so that we're able to close the file handle per process/thread when we're done with the SHM.
| shm->shm_nattch = 0; | ||
| if (NULL != shm->segment && INVALID_HANDLE_VALUE != shm->segment && FALSE == shm->created) { | ||
| if(NULL != shm->descriptor){ | ||
| ret = UnmapViewOfFile(shm->descriptor) ? 0 : -1; |
There was a problem hiding this comment.
I still think that we need to UnmapViewOfFile() "unconditionally". From https://learn.microsoft.com/en-us/windows/win32/api/winbase/nf-winbase-createfilemappinga:
Therefore, to fully close a file mapping object, an application must unmap all mapped views of the file mapping object by calling UnmapViewOfFile and close the file mapping object handle by calling CloseHandle.
If we only unmap when the number of attachments drops to zero, that would not necessarily match the attachments.
|
Not particularly related to this PR, but more generally, in my opinion we need way more relevant tests for our System V shared memory on Windows compatibility layer. I'm not sure if we can have integration tests; https://learn.microsoft.com/en-us/sysinternals/downloads/process-explorer allows to see file mappings, but it likely uses some undocumented low-level APIs, and the mappings may not be removed right away after closing anyway. Maybe some unit tests would be in order (might be hard to accomplish, though). |
Fixed work with Shared Memory on Windows.