🤗 [Question]: Is there a concurrency safety issue in the Set method implementation of Minio Storage?

[ilxqx]

Question Description

Please take a look at the code below:

s.cfg.PutObjectOptions.ContentType = http.DetectContentType(val)

Are there any concurrency safety issues here?
Here is a more complete code snippet below.

Code Snippet (optional)

// Set key with value
func (s *Storage) Set(key string, val []byte, exp time.Duration) error {

	if len(key) <= 0 {
		return errors.New("the key value is required")
	}

	// create Reader
	file := bytes.NewReader(val)

	// set content type
        // Note: In general, the Storage object is a global singleton. Is there any issue with modifying its internal properties here? If multiple people concurrently upload files, will there be a problem of ContentType mix-up?
	s.cfg.PutObjectOptions.ContentType = http.DetectContentType(val)

	// put object
	_, err := s.minio.PutObject(s.ctx, s.cfg.Bucket, key, file, file.Size(), s.cfg.PutObjectOptions)

	return err
}

Checklist:

• I agree to follow Fiber's Code of Conduct.
• I have checked for existing issues that describe my questions prior to opening this one.
• I understand that improperly formatted questions may be closed without explanation.

[gaby]

Ping @mstgnz

[mstgnz]

DetectContentType is not stated to be thread-safe in the Go documentation, so updating ContentType synchronously without proper synchronization can lead to unexpected behavior.

In fact, Minio sets the Content Type automatically, but this is not 100% accurate. That's why we set it ourselves.

[ilxqx]

@mstgnz Thank you for your reply. I think we should use the copy technique here. What do you think?

// If two coroutines arrive here simultaneously, one is a request to upload an image and the other is a request to upload a docx file.

Assuming the first Coroutine acquired the lock and changed PutObjectOptions.ContentType to image/png, but before it could proceed to the next line of code, the CPU relinquished control.

Then the second Coroutine starts acquiring the lock and changes PutObjectOptions.ContentType to application/octet, releases the lock, and prepares to execute the next line of code.

Now, regardless of when the first Coroutine is resumed, it is highly likely that the PutObjectOptions.ContentType used in executing the PutObject function may be incorrect.

        file := bytes.NewReader(val)

	// If two coroutines arrive here simultaneously, one is a request to upload an image and the other is a request to upload a docx file.
        // Assuming the first Coroutine acquired the lock and changed PutObjectOptions.ContentType to "image/png", but before it could proceed to the next line of code, the CPU relinquished control.
        // Then the second Coroutine starts acquiring the lock and changes PutObjectOptions.ContentType to application/octet, releases the lock, and prepares to execute the next line of code.
        // Now, regardless of when the first Coroutine is resumed, it is highly likely that the PutObjectOptions.ContentType used in executing the PutObject function may be incorrect.
	s.mu.Lock()
	s.cfg.PutObjectOptions.ContentType = http.DetectContentType(val)
	s.mu.Unlock()

	// put object
	_, err := s.minio.PutObject(s.ctx, s.cfg.Bucket, key, file, file.Size(), s.cfg.PutObjectOptions)

One approach is to expand the scope of locking, but this can significantly reduce execution efficiency:

        file := bytes.NewReader(val)

	s.mu.Lock()
	s.cfg.PutObjectOptions.ContentType = http.DetectContentType(val)
	_, err := s.minio.PutObject(s.ctx, s.cfg.Bucket, key, file, file.Size(), s.cfg.PutObjectOptions)
	s.mu.Unlock()

The above is my limited understanding, and there may be misunderstandings. I look forward to being corrected.

[mstgnz]

Although it is not entirely clear, minio PutObject is said to be safe for goroutines. However, a situation like the one you mentioned is theoretically possible, so we need to ensure our security ourselves. Thanks for the information. Related correction made

There is a minio issue related to this situation. minio/minio-go#598

[gaby]

@mstgnz Are there more locks needed in #1144 ?

[mstgnz]

@gaby No, there's no more.

[ReneWerner87]

released https://github.com/gofiber/storage/releases/tag/minio%2Fv0.2.1