[fs] Add the file.close method to the fs module (3/3)

[oleiade]

What?

This Pull Request builds (and is based) upon #3142 and adds a close method to the File object exposed by the k6/experimental/fs module.

The close method currently does not perform any operations. This is intentional.

Why?

Rationale Behind No-Op Close Method:

• User Expectations: During our design discussions, we identified that users would likely anticipate the presence of a close method for consistency. Thus, we're aligning with these expectations.
• Futureproofing: Although the method is a no-op now, we might consider enriching its functionality in the future.

Why Not Implement Functionality Now?

Certainly, functionality can be added. As mentioned in #3142, we maintain a file cache that users access through File instance pointers. This ensures a more efficient memory utilization across various VUs, compared to our existing open method.

Envision a scenario where only one out of several user scenarios opens a file. In such a case, tracking how many VUs haven't closed the file (via reference counting) becomes feasible. When the count hits zero, the file content can be purged from memory.

I've crafted a proof of concept for this approach. However, a current k6 limitation hinders its realization. Presently, k6 doesn't support opening files within the VU context (refer to #3020). This implies that even with reference counting via file.close, we'd always have an uncollected reference, preventing the count from reaching zero.

Checklist

• I have performed a self-review of my code.
• I have added tests for my changes.
• I have run linter locally (make ci-like-lint) and all checks pass.
• I have run tests locally (make tests) and all tests pass.
• I have commented on my code, particularly in hard-to-understand areas.

Related PR(s)/Issue(s)

ref #3142
ref #3149
ref #3165

[codecov-commenter]

Codecov Report

All modified lines are covered by tests ✅

❗ No coverage uploaded for pull request base (experimental/fs-readseek@0a4f80b). Click here to learn what that means.

❗ Current head 30d2a2b differs from pull request most recent head 706a61c. Consider uploading reports for the commit 706a61c to get more accurate results

Additional details and impacted files

@@                     Coverage Diff                     @@
##             experimental/fs-readseek    #3314   +/-   ##
===========================================================
  Coverage                            ?   73.07%           
===========================================================
  Files                               ?      262           
  Lines                               ?    20125           
  Branches                            ?        0           
===========================================================
  Hits                                ?    14706           
  Misses                              ?     4470           
  Partials                            ?      949           

Flag	Coverage Δ
windows	73.07% <0.00%> (?)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[codebien]

This implies that even with reference counting via file.close, we'd always have an uncollected reference, preventing the count from reaching zero.

@oleiade We are able to have k6/execution.vu.idInTest so we should already have a function for getting the accurate counter. Can you explain more, please?

[oleiade]

Hey @codebien 👋🏻

I've spent a couple of hours more on my ref-counting POC to see if there was an alternative, as you suggested, to make sure we would release the memory once we don't need it anymore.

I made some progress and was able to release the memory as soon as all VUs referencing it were done indeed.

However, I encountered another issue, which I'm not sure can be addressed, nor if it should: handleSummary evaluates the init context, which in the current context will make the module reload the file in memory if it's not present yet. Effectively defeating the initial goal of reference counting.

Eventually, once we have the ability to open files in the VU stage as per the design proposal I submitted internally, it would be worth attempting to resume this approach (if the file is opened in the VU stage, the handleSummary wouldn't expect to have access to it, and with a VU id based reference counting, we would probably be able to clear the file from memory safely). But I think it's not worth it in the current state of things.

What do you think? Do you maybe have other approaches that might be worth considering?

[codebien]

Just as a reminder note. We have discussed it internally and we've set a potential solution. @oleiade will write a proposal for integrating the current design doc.

[codebien]

In a distributed mode, what should the expectation be? Teardown runs only once per Test instead of the init context that tuns once per VU.

[codebien]

Suggested change

	f.closed.Store(true)
	f.closed.Store(true)
	f.data = nil

I think here we may release the pointer. We don't expect to access them anymore, and in the bad case, there is an attempt we can just check the closed variable and return an error saying the file has been closed.

[oleiade]

We have discussed this feature (fs.close) in depth with @codebien, and concluded that in the current state of things, we prefer not to merge this and delay introducing the fs.close feature to a later point.