Add io/fs FS support

[Aoang]

No description provided.

[Aoang]

RFC @erikdubbelboer

[erikdubbelboer]

I'm ok with dropping support for 1.15.

I would start without compress. Shouldn't the index page and byte-range work without any change since it's still just "files"? And I would add support for compression with in-memory cache after that.

[efectn]

I think you can implement fs.FS instead of embed.FS. It's more useful for general purpose.

[Aoang]

I think you can implement fs.FS instead of embed.FS. It's more useful for general purpose.

LGMT. I'm doing and sharing my thoughts.

We first need to implement the Custom FS interface based on os.File to be compatible with the current API. Then complete fs.FS and embed.FS.

For some FS, we may need to self-implement io.ReadAt io.Seeker and more.

The challenge lies in how to elegantly implement the compression with in-memory or in-file cache. Oh, and zero alloc.

Of course, it may takes a while to complete it.

According to this line of thinking, compatibility existing APIs need a great care, and more tests need to be added.

Zero alloc and elegant code are like tongues and teeth, that sometimes collide.

[efectn]

I think you can implement fs.FS instead of embed.FS. It's more useful for general purpose.

LGMT. I'm doing and sharing my thoughts.

We first need to implement the Custom FS interface based on os.File to be compatible with the current API. Then complete fs.FS and embed.FS.

For some FS, we may need to self-implement io.ReadAt io.Seeker and more.

The challenge lies in how to elegantly implement the compression with in-memory or in-file cache. Oh, and zero alloc.

Of course, it may takes a while to complete it.

According to this line of thinking, compatibility existing APIs need a great care, and more tests need to be added.

Zero alloc and elegant code are like tongues and teeth, that sometimes collide.

Exactly agree. I'm also not sure about conpression with fs.Fs

[Aoang]

I need some help.

https://github.com/valyala/fasthttp/blob/v1.40.0/fs.go#L822-L826

I don't quite understand why byte(0) needs to be handled separately. Does Linux, other systems or file systems have special handling? If handle exceptions, should handle all invisible characters, shouldn't you?

Going back to the source.
6a748e6#diff-a15ccbc05dcabeeea9949bb6a0cdb656ca2f12b96dfbcdd9fe4199ca65a3bcafR153-R157

I found that it already existed when it was created, can take a moment to answer it? @valyala

[kirillDanshin]

@Aoang null byte is a string delimiter. if we see it in the path, it means it was maliciously injected, and we need not to serve such request for security reasons. if we're talking Go in general, there's no realistic cases when a path would have a null byte, especially mid-string, as Go doesn't use null bytes to indicate the end of the string, instead it just stores the string length.

[Aoang]

Thanks.

https://en.wikipedia.org/wiki/Null_character

[Aoang]

I'm confused, sync.WaitGroup fail in race tests.

https://github.com/valyala/fasthttp/actions/runs/3203507569/jobs/5233684491#step:8:6
bfce8ab

[erikdubbelboer]

I'm not sure I understand what the WaitGroup is for. But I think this happens when Add is called after Wait.

[Aoang]

Test

1. New Go File: wg.go

package testPoj

import (
	"sync"
	"time"
)

var wg = sync.WaitGroup{}

func R() {
	wg.Add(5)
	for i := 0; i < 5; i++ {
		run()
	}
	wg.Wait()
}

func run() {
	go func() {
		wg.Done()
		wg.Add(1)
		time.Sleep(time.Second)
		wg.Done()
	}()
}

2. New Go File: wg_test.go

package testPoj

import (
	"testing"
)

func TestR(t *testing.T) {
	R()
}

3. Run go test -v -race ./...
4. Output:

➜  go test -v -race ./...
=== RUN   TestR
--- PASS: TestR (1.01s)
PASS
ok      testPoj 2.053s

Could this test be inaccurate? I‘ll try again tomorrow.

[erikdubbelboer]

What if you have multiple goroutines calling wg.Wait()? I can't imagine WaitGroup has any bugs in it.

[Aoang]

Hi, in the implementation process, the package in internal depends on compress, so I copied a copy in internal/pool/compress.

There are so many exposed methods and variables in compress that it is not appropriate to put it in internal?

Should create a compress package like stackless?

> tree
.
├── allocation_test.go
├── coarseTime.go
├── coarseTime_test.go
├── compress [new package]
│   ├── compress.go
│   └── compress_test.go
├── compress.go [for compatibility, and marked as deprecated]
...

[erikdubbelboer]

Is there so much code that an internal package is really needed? I was hoping it would all fit into something like fs_embed.go in the root.

[Aoang]

I don't think it is necessary, but there are too many duplicate names, such as io.FS and embed.FS.

I think it would be a lot easier to add embed.FS and io.FS functionality without making too many changes to the original code. You can copy a fsFSHandler and change the cache to a memory cache. Then, copy the code for the other logic and make some minor modifications to complete it.

Do you think this is better?

[erikdubbelboer]

Hard to judge without diving deep into it.

Isn't it possible to add an embed.FS to fasthttp.FS and use that to list dirs and open files if provided. For caching you then just do that in memory. Then compress etc can stay exactly the same.

What do you mean with io.FS?

[Aoang]

To be reviewed, I'll add comments.

[Aoang]

No progress? close it

[efectn]

Why to close it? I think this would be great feature

[byene0923]

why close？need some help?

[piyongcai]

没有进展？关闭它

@Aoang Why Close？

[Aoang]

If you're interested in continuing this work, this patch might help. Based on it, then also need to add ByteRange support, maybe more.