fix: Implementing a limitation for the open() #2314
Conversation
|
Thanks, @mstoykov 🙇 Addressed all comments 👍 |
js/initcontext.go
Outdated
| defer func() { | ||
| if errors.Is(err, fsext.ErrPathNeverRequestedBefore) { | ||
| // loading different files per VU is not supported, so all files should are going | ||
| // to be used inside the scenario should be opened during the init step (without any conditions) | ||
| err = fmt.Errorf( | ||
| "open() can't be used under with files that weren't opened during initialization (__VU==0), path: %q", | ||
| filename, | ||
| ) | ||
| } | ||
| }() |
There was a problem hiding this comment.
Note(not actionable): Technically, this error will either happen on afero.IsDir or not at all. But it seems kind of strange to check it only there and also when/if we ever stop checking that as we move to io/fs it will be a bit confusing why it wasn't previously checked.
I am noting this for future reference only, don't do anything about it just based on that.
There was a problem hiding this comment.
🤔 not sure if I get it. The error happens afero.IsDir and afero.ReadFile. Even if we move to the io/fs still should happen on the equivalent of the afero.ReadFile, no?
There was a problem hiding this comment.
When we move – yes, but when we move to io/fs, afero.IsDir will hopefully be just dropped ;).
What I am trying to say is that if afero.IsDir did not return fsext.ErrPathNeverRequestedBefore than afero.Readfile will not return it either ... or we have bugs I guess or some strange race condition.
I wondered for a bit if we can't just add afero.IsDir check in the already reimplemented Open , but I am not certain we always use the CacheOnReadFs and IIRC we don't so 🤷
js/initcontext.go
Outdated
| fs := i.filesystems["file"] | ||
|
|
||
| alreadyOpenedFS, ok := fs.(fsext.OnlyOpenedEnabler) | ||
| if !ok { | ||
| return | ||
| } | ||
|
|
||
| alreadyOpenedFS.AllowOnlyOpened() | ||
| } |
There was a problem hiding this comment.
Note(not actionable): This seems a bit fragile, as we can at any point change what the top fs for file is and then this suddenly won't work. This is unlikely as this code is very rarely changed and will probably be spotted.
The test below also doesn't test with loader.CreateFilesystems which is what is used in reality. An integration test with it might be better, but I see no test actually has ever used it so 🤷 maybe there are some problems I can't remember at the moment. Either way this is fine for now, hopefully we will be moving to io/fs soon and that will lead to some bigger changes and hopefully more tests.
olegbespalov
added
breaking change
olegbespalov
force-pushed
the
fix/1771-fixes
branch
from
fe37712 to
aeb4217
Compare
Implementing a limitation for the open() that limits opening files
by the list of the files that were opened during the initialization
step (__VU == 0).
For example, a code like:
```js
if (__VU >0) {
JSON.parse(open("./arr.json"));
}
```
Should return an error.
Closes #1771
Co-authored-by: Mihail Stoykov <[email protected]>
olegbespalov
force-pushed
the
fix/1771-fixes
branch
from
aeb4217 to
58792b3
Compare
Implementing a limitation for the open() that limits opening files
by the list of the files that were opened during the initialization
step (__VU == 0).
For example, a code like:
Should return an error.
Closes #1771