Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix Truncated ArchiveCacheMethod #30

Closed
wants to merge 2 commits into from
Closed

Fix Truncated ArchiveCacheMethod #30

wants to merge 2 commits into from

Conversation

dlamkins
Copy link
Contributor

@dlamkins dlamkins commented Jan 24, 2020 •
edited
Loading

Currently when flushed, the ArchiveCacheMethod writes the archive to disk by disposing of the archive and archiveStream. Once done, it reopens the archive so that it can be used for more caching. Currently when reopening, the FileMode is set to Truncate immediately clearing the archive file that was just created as a result of the flush.

This PR just changes it to OpenOrCreate as it is when the ArchiveCacheMethod is first instanced.

Intended to fix #29

@dlamkins
Copy link
Contributor Author

I am confused by this. Am I misunderstanding what the flush is intended to do? Is it saying it expects the entry to be null?

Test run for D:\a\Gw2Sharp\Gw2Sharp\Gw2Sharp.Tests\bin\Release\netcoreapp3.0\Gw2Sharp.Tests.dll(.NETCoreApp,Version=v3.0)
Microsoft (R) Test Execution Command Line Tool Version 16.3.0
Copyright (c) Microsoft Corporation.  All rights reserved.

Starting test execution, please wait...

A total of 1 test files matched the specified pattern.
[xUnit.net 00:00:11.11]     Gw2Sharp.Tests.WebApi.Caching.ArchiveCacheMethodTests.FlushTest [FAIL]
  X Gw2Sharp.Tests.WebApi.Caching.ArchiveCacheMethodTests.FlushTest [7ms]
  Error Message:
   Assert.Null() Failure
Expected: (null)
Actual:   CacheItem`1 { Category = "Test category", ExpiryTime = 2020-01-24T04:00:28.4896979+00:00, Id = "test", Item = 42, Item = 42 }
  Stack Trace:
     at Gw2Sharp.Tests.WebApi.Caching.BaseCacheMethodTests.FlushTest() in D:\a\Gw2Sharp\Gw2Sharp\Gw2Sharp.Tests\WebApi\Caching\BaseCacheMethodTests.cs:line 30
--- End of stack trace from previous location where exception was thrown ---

Test Run Failed.
Total tests: 1006
     Passed: 1005
     Failed: 1
 Total time: 21.4329 Seconds
##[error]Process completed with exit code 1.

@Archomeda
Copy link
Owner

Archomeda commented Jan 25, 2020 •
edited
Loading

I checked the issue, and what the test is expecting, is correct. In hindsight, I've just named that method badly.
It probably didn't come to mind that Flush is actually flushing the internal cache to write it to disk in FileStream terms, because this interface was written before I even thought of writing the ArchiveCacheMethod (the MemoryCacheMethod was the only class that implemented it).

I'll make a note to rename it in version 0.9.0+ while making it deprecated in 0.8.0.

Regarding why it supposedly fails as mentioned in #29, I actually can't pinpoint the issue. I'll update your branch with an additional test to make sure to check if the ArchiveCacheMethod contains the cache correctly after the stream is disposed and reopened for non-raw data that needs to be serialized and deserialized. Right now it's only checking raw byte arrays.

Archomeda
Archomeda reviewed Jan 25, 2020
View reviewed changes
Copy link
Owner

@Archomeda Archomeda left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've added the additional test. You'll have to revert your change back to Truncate since that's the actual intent.

Since I still can't find the issue and that my tests are succeeding (after reverting your change, but I left that alone in my commit), maybe you're able to find the original issue now that you have more information?

Oh, and I also updated master to use .NET Core 3.1.101 for CI and changed the required checks. You'll have to update your branch from master to have them picked up properly.

@Archomeda Archomeda mentioned this pull request Jan 25, 2020
Closed
@Archomeda Archomeda added Area: Web cache Related to the web API caching Status: Discussion needed Extra discussion is needed labels Jan 25, 2020
@Archomeda
Copy link
Owner

Because #29 hasn't been an issue for a while, I'm closing this. If there's still a problem, feel free to open a new issue/PR.

@Archomeda Archomeda closed this Mar 21, 2020
@Archomeda Archomeda removed the Status: Discussion needed Extra discussion is needed label Mar 21, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Archomeda Archomeda Archomeda left review comments

Assignees
No one assigned
Labels
Area: Web cache Related to the web API caching
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Archive caching results in empty ZIP files
2 participants
@dlamkins @Archomeda