tus / tusd

Reference server implementation in Go of tus: the open protocol for resumable file uploads
https://tus.github.io/tusd
MIT License
2.95k stars 467 forks source link

Notify*Uploads in Config and handler.*Uploads Are Public and May Conflict With Internal Code #1007

Closed jimydavis closed 9 months ago

jimydavis commented 9 months ago
go func() {
  for {
    event := <-tusdHandler.CreatedUploads
    log.Info().Msg(fmt.Sprintf("Upload %s Created.", event.Upload.ID))
  }
}()

What then happened was the code in hooks/hooks.go had a race condition on consuming the handler.CreatedUploads channel.

case event := <-handler.CreatedUploads:
  invokeHookAsync(HookPostCreate, event, hookHandler)

Effectively, sometimes the post-create hook was notified and sometimes it was not. After I removed my own monitoring code that was consuming the channel, everything worked as intended. Since this config is public, a user wouldn't know that there might be such a race condition.

Am I interpreting this correctly?

If this is indeed an issue, a suggested solution is that of:

  1. a broadcaster pattern or a broker pattern: https://stackoverflow.com/questions/36417199/how-to-broadcast-message-using-channel OR
  2. create a different line of communication for monitoring vs. hook invocation . I think your "Metrics" package is trying to do some monitoring but I haven't read or used it yet so maybe there is no need for separate monitoring.
Acconut commented 9 months ago

You are right, that is a case that did not arose before the hooks implementation was moved to a public package. I will add some documentation saying if NewHandlerWithHooks is used, the notification channels must not be used anymore.