From d40f54dc9df0da031d3c4e95a498f9d1e514d86d Mon Sep 17 00:00:00 2001 From: Robert Swain Date: Tue, 6 Jun 2017 00:49:15 +0200 Subject: [PATCH] mediaapi/thumbnailer: Limit number of parallel generators Fall back to selecting from already-/pre-generated thumbnails or serving the original. --- .../cmd/dendrite-media-api-server/main.go | 26 ++++--- .../dendrite/mediaapi/config/config.go | 2 + .../mediaapi/thumbnailer/thumbnailer.go | 68 ++++++++++++------- .../dendrite/mediaapi/writers/download.go | 39 +++++++---- .../dendrite/mediaapi/writers/upload.go | 9 ++- 5 files changed, 93 insertions(+), 51 deletions(-) diff --git a/src/github.com/matrix-org/dendrite/cmd/dendrite-media-api-server/main.go b/src/github.com/matrix-org/dendrite/cmd/dendrite-media-api-server/main.go index 2ce9226da..e50179374 100644 --- a/src/github.com/matrix-org/dendrite/cmd/dendrite-media-api-server/main.go +++ b/src/github.com/matrix-org/dendrite/cmd/dendrite-media-api-server/main.go @@ -70,15 +70,16 @@ func main() { } log.WithFields(log.Fields{ - "BIND_ADDRESS": bindAddr, - "LOG_DIR": logDir, - "CONFIG_PATH": configPath, - "ServerName": cfg.ServerName, - "AbsBasePath": cfg.AbsBasePath, - "MaxFileSizeBytes": *cfg.MaxFileSizeBytes, - "DataSource": cfg.DataSource, - "DynamicThumbnails": cfg.DynamicThumbnails, - "ThumbnailSizes": cfg.ThumbnailSizes, + "BIND_ADDRESS": bindAddr, + "LOG_DIR": logDir, + "CONFIG_PATH": configPath, + "ServerName": cfg.ServerName, + "AbsBasePath": cfg.AbsBasePath, + "MaxFileSizeBytes": *cfg.MaxFileSizeBytes, + "DataSource": cfg.DataSource, + "DynamicThumbnails": cfg.DynamicThumbnails, + "MaxThumbnailGenerators": cfg.MaxThumbnailGenerators, + "ThumbnailSizes": cfg.ThumbnailSizes, }).Info("Starting mediaapi server with configuration") routing.Setup(http.DefaultServeMux, http.DefaultClient, cfg, db) @@ -167,6 +168,13 @@ func applyOverrides(cfg *config.MediaAPI) { } cfg.DataSource = dataSource } + + if cfg.MaxThumbnailGenerators == 0 { + log.WithField( + "max_thumbnail_generators", cfg.MaxThumbnailGenerators, + ).Info("Using default max_thumbnail_generators") + cfg.MaxThumbnailGenerators = 10 + } } func validateConfig(cfg *config.MediaAPI) error { diff --git a/src/github.com/matrix-org/dendrite/mediaapi/config/config.go b/src/github.com/matrix-org/dendrite/mediaapi/config/config.go index 069befb00..14c12e861 100644 --- a/src/github.com/matrix-org/dendrite/mediaapi/config/config.go +++ b/src/github.com/matrix-org/dendrite/mediaapi/config/config.go @@ -35,6 +35,8 @@ type MediaAPI struct { DataSource string `yaml:"database"` // Whether to dynamically generate thumbnails on-the-fly if the requested resolution is not already generated DynamicThumbnails bool `yaml:"dynamic_thumbnails"` + // The maximum number of simultaneous thumbnail generators. default: 10 + MaxThumbnailGenerators int `yaml:"max_thumbnail_generators"` // A list of thumbnail sizes to be pre-generated for downloaded remote / uploaded content ThumbnailSizes []types.ThumbnailSize `yaml:"thumbnail_sizes"` } diff --git a/src/github.com/matrix-org/dendrite/mediaapi/thumbnailer/thumbnailer.go b/src/github.com/matrix-org/dendrite/mediaapi/thumbnailer/thumbnailer.go index 0bc7a06ff..4d0d39a5d 100644 --- a/src/github.com/matrix-org/dendrite/mediaapi/thumbnailer/thumbnailer.go +++ b/src/github.com/matrix-org/dendrite/mediaapi/thumbnailer/thumbnailer.go @@ -40,39 +40,47 @@ type thumbnailFitness struct { const thumbnailTemplate = "thumbnail-%vx%v-%v" // GenerateThumbnails generates the configured thumbnail sizes for the source file -func GenerateThumbnails(src types.Path, configs []types.ThumbnailSize, mediaMetadata *types.MediaMetadata, activeThumbnailGeneration *types.ActiveThumbnailGeneration, db *storage.Database, logger *log.Entry) error { +func GenerateThumbnails(src types.Path, configs []types.ThumbnailSize, mediaMetadata *types.MediaMetadata, activeThumbnailGeneration *types.ActiveThumbnailGeneration, maxThumbnailGenerators int, db *storage.Database, logger *log.Entry) (busy bool, errorReturn error) { buffer, err := bimg.Read(string(src)) if err != nil { logger.WithError(err).WithField("src", src).Error("Failed to read src file") - return err + return false, err } for _, config := range configs { // Note: createThumbnail does locking based on activeThumbnailGeneration - if err = createThumbnail(src, buffer, config, mediaMetadata, activeThumbnailGeneration, db, logger); err != nil { + busy, err = createThumbnail(src, buffer, config, mediaMetadata, activeThumbnailGeneration, maxThumbnailGenerators, db, logger) + if err != nil { logger.WithError(err).WithField("src", src).Error("Failed to generate thumbnails") - return err + return false, err + } + if busy { + return true, nil } } - return nil + return false, nil } // GenerateThumbnail generates the configured thumbnail size for the source file -func GenerateThumbnail(src types.Path, config types.ThumbnailSize, mediaMetadata *types.MediaMetadata, activeThumbnailGeneration *types.ActiveThumbnailGeneration, db *storage.Database, logger *log.Entry) error { +func GenerateThumbnail(src types.Path, config types.ThumbnailSize, mediaMetadata *types.MediaMetadata, activeThumbnailGeneration *types.ActiveThumbnailGeneration, maxThumbnailGenerators int, db *storage.Database, logger *log.Entry) (busy bool, errorReturn error) { buffer, err := bimg.Read(string(src)) if err != nil { logger.WithError(err).WithFields(log.Fields{ "src": src, }).Error("Failed to read src file") - return err + return false, err } // Note: createThumbnail does locking based on activeThumbnailGeneration - if err = createThumbnail(src, buffer, config, mediaMetadata, activeThumbnailGeneration, db, logger); err != nil { + busy, err = createThumbnail(src, buffer, config, mediaMetadata, activeThumbnailGeneration, maxThumbnailGenerators, db, logger) + if err != nil { logger.WithError(err).WithFields(log.Fields{ "src": src, }).Error("Failed to generate thumbnails") - return err + return false, err } - return nil + if busy { + return true, nil + } + return false, nil } // GetThumbnailPath returns the path to a thumbnail given the absolute src path and thumbnail size configuration @@ -130,7 +138,7 @@ func SelectThumbnail(desired types.ThumbnailSize, thumbnails []*types.ThumbnailM // createThumbnail checks if the thumbnail exists, and if not, generates it // Thumbnail generation is only done once for each non-existing thumbnail. -func createThumbnail(src types.Path, buffer []byte, config types.ThumbnailSize, mediaMetadata *types.MediaMetadata, activeThumbnailGeneration *types.ActiveThumbnailGeneration, db *storage.Database, logger *log.Entry) (errorReturn error) { +func createThumbnail(src types.Path, buffer []byte, config types.ThumbnailSize, mediaMetadata *types.MediaMetadata, activeThumbnailGeneration *types.ActiveThumbnailGeneration, maxThumbnailGenerators int, db *storage.Database, logger *log.Entry) (busy bool, errorReturn error) { logger = logger.WithFields(log.Fields{ "Width": config.Width, "Height": config.Height, @@ -140,9 +148,12 @@ func createThumbnail(src types.Path, buffer []byte, config types.ThumbnailSize, dst := GetThumbnailPath(src, config) // Note: getActiveThumbnailGeneration uses mutexes and conditions from activeThumbnailGeneration - isActive, err := getActiveThumbnailGeneration(dst, config, activeThumbnailGeneration, logger) + isActive, busy, err := getActiveThumbnailGeneration(dst, config, activeThumbnailGeneration, maxThumbnailGenerators, logger) if err != nil { - return err + return false, err + } + if busy { + return true, nil } if isActive { @@ -162,28 +173,28 @@ func createThumbnail(src types.Path, buffer []byte, config types.ThumbnailSize, thumbnailMetadata, err := db.GetThumbnail(mediaMetadata.MediaID, mediaMetadata.Origin, config.Width, config.Height, config.ResizeMethod) if err != nil { logger.Error("Failed to query database for thumbnail.") - return err + return false, err } if thumbnailMetadata != nil { - return nil + return false, nil } // Note: The double-negative is intentional as os.IsExist(err) != !os.IsNotExist(err). // The functions are error checkers to be used in different cases. if _, err = os.Stat(string(dst)); !os.IsNotExist(err) { // Thumbnail exists - return nil + return false, nil } if isActive == false { // Note: This should not happen, but we check just in case. logger.Error("Failed to stat file but this is not the active thumbnail generator. This should not happen.") - return fmt.Errorf("Not active thumbnail generator. Stat error: %q", err) + return false, fmt.Errorf("Not active thumbnail generator. Stat error: %q", err) } start := time.Now() width, height, err := resize(dst, buffer, config.Width, config.Height, config.ResizeMethod == "crop", logger) if err != nil { - return err + return false, err } logger.WithFields(log.Fields{ "ActualWidth": width, @@ -193,7 +204,7 @@ func createThumbnail(src types.Path, buffer []byte, config types.ThumbnailSize, stat, err := os.Stat(string(dst)) if err != nil { - return err + return false, err } thumbnailMetadata = &types.ThumbnailMetadata{ @@ -217,14 +228,14 @@ func createThumbnail(src types.Path, buffer []byte, config types.ThumbnailSize, "ActualWidth": width, "ActualHeight": height, }).Error("Failed to store thumbnail metadata in database.") - return err + return false, err } - return nil + return false, nil } // getActiveThumbnailGeneration checks for active thumbnail generation -func getActiveThumbnailGeneration(dst types.Path, config types.ThumbnailSize, activeThumbnailGeneration *types.ActiveThumbnailGeneration, logger *log.Entry) (bool, error) { +func getActiveThumbnailGeneration(dst types.Path, config types.ThumbnailSize, activeThumbnailGeneration *types.ActiveThumbnailGeneration, maxThumbnailGenerators int, logger *log.Entry) (isActive bool, busy bool, errorReturn error) { // Check if there is active thumbnail generation. activeThumbnailGeneration.Lock() defer activeThumbnailGeneration.Unlock() @@ -234,7 +245,14 @@ func getActiveThumbnailGeneration(dst types.Path, config types.ThumbnailSize, ac // NOTE: Wait unlocks and locks again internally. There is still a deferred Unlock() that will unlock this. activeThumbnailGenerationResult.Cond.Wait() // Note: either there is an error or it is nil, either way returning it is correct - return false, activeThumbnailGenerationResult.Err + return false, false, activeThumbnailGenerationResult.Err + } + + // Only allow thumbnail generation up to a maximum configured number. Above this we fall back to serving the + // original. Or in the case of pre-generation, they maybe get generated on the first request for a thumbnail if + // load has subsided. + if len(activeThumbnailGeneration.PathToResult) >= maxThumbnailGenerators { + return false, true, nil } // No active thumbnail generation so create one @@ -242,7 +260,7 @@ func getActiveThumbnailGeneration(dst types.Path, config types.ThumbnailSize, ac Cond: &sync.Cond{L: activeThumbnailGeneration}, } - return true, nil + return true, false, nil } // broadcastGeneration broadcasts that thumbnail generation completed and the error to all waiting goroutines @@ -252,7 +270,7 @@ func broadcastGeneration(dst types.Path, activeThumbnailGeneration *types.Active defer activeThumbnailGeneration.Unlock() if activeThumbnailGenerationResult, ok := activeThumbnailGeneration.PathToResult[string(dst)]; ok { logger.Info("Signalling other goroutines waiting for this goroutine to generate the thumbnail.") - // Note: retErr is a named return value error that is signalled from here to waiting goroutines + // Note: errorReturn is a named return value error that is signalled from here to waiting goroutines activeThumbnailGenerationResult.Err = errorReturn activeThumbnailGenerationResult.Cond.Broadcast() } diff --git a/src/github.com/matrix-org/dendrite/mediaapi/writers/download.go b/src/github.com/matrix-org/dendrite/mediaapi/writers/download.go index 2b3b432fc..90f6ca35b 100644 --- a/src/github.com/matrix-org/dendrite/mediaapi/writers/download.go +++ b/src/github.com/matrix-org/dendrite/mediaapi/writers/download.go @@ -192,12 +192,12 @@ func (r *downloadRequest) doDownload(w http.ResponseWriter, cfg *config.MediaAPI // If we have a record, we can respond from the local file r.MediaMetadata = mediaMetadata } - return r.respondFromLocalFile(w, cfg.AbsBasePath, activeThumbnailGeneration, db, cfg.DynamicThumbnails, cfg.ThumbnailSizes) + return r.respondFromLocalFile(w, cfg.AbsBasePath, activeThumbnailGeneration, cfg.MaxThumbnailGenerators, db, cfg.DynamicThumbnails, cfg.ThumbnailSizes) } // respondFromLocalFile reads a file from local storage and writes it to the http.ResponseWriter // Returns a util.JSONResponse error in case of error -func (r *downloadRequest) respondFromLocalFile(w http.ResponseWriter, absBasePath types.Path, activeThumbnailGeneration *types.ActiveThumbnailGeneration, db *storage.Database, dynamicThumbnails bool, thumbnailSizes []types.ThumbnailSize) *util.JSONResponse { +func (r *downloadRequest) respondFromLocalFile(w http.ResponseWriter, absBasePath types.Path, activeThumbnailGeneration *types.ActiveThumbnailGeneration, maxThumbnailGenerators int, db *storage.Database, dynamicThumbnails bool, thumbnailSizes []types.ThumbnailSize) *util.JSONResponse { filePath, err := fileutils.GetPathFromBase64Hash(r.MediaMetadata.Base64Hash, absBasePath) if err != nil { r.Logger.WithError(err).Error("Failed to get file path from metadata") @@ -230,7 +230,7 @@ func (r *downloadRequest) respondFromLocalFile(w http.ResponseWriter, absBasePat var responseFile *os.File var responseMetadata *types.MediaMetadata if r.IsThumbnailRequest { - thumbFile, thumbMetadata, resErr := r.getThumbnailFile(types.Path(filePath), activeThumbnailGeneration, db, dynamicThumbnails, thumbnailSizes) + thumbFile, thumbMetadata, resErr := r.getThumbnailFile(types.Path(filePath), activeThumbnailGeneration, maxThumbnailGenerators, db, dynamicThumbnails, thumbnailSizes) if thumbFile != nil { defer thumbFile.Close() } @@ -284,16 +284,19 @@ func (r *downloadRequest) respondFromLocalFile(w http.ResponseWriter, absBasePat } // Note: Thumbnail generation may be ongoing asynchronously. -func (r *downloadRequest) getThumbnailFile(filePath types.Path, activeThumbnailGeneration *types.ActiveThumbnailGeneration, db *storage.Database, dynamicThumbnails bool, thumbnailSizes []types.ThumbnailSize) (*os.File, *types.ThumbnailMetadata, *util.JSONResponse) { +func (r *downloadRequest) getThumbnailFile(filePath types.Path, activeThumbnailGeneration *types.ActiveThumbnailGeneration, maxThumbnailGenerators int, db *storage.Database, dynamicThumbnails bool, thumbnailSizes []types.ThumbnailSize) (*os.File, *types.ThumbnailMetadata, *util.JSONResponse) { var thumbnail *types.ThumbnailMetadata var resErr *util.JSONResponse if dynamicThumbnails { - thumbnail, resErr = r.generateThumbnail(filePath, r.ThumbnailSize, activeThumbnailGeneration, db) + thumbnail, resErr = r.generateThumbnail(filePath, r.ThumbnailSize, activeThumbnailGeneration, maxThumbnailGenerators, db) if resErr != nil { return nil, nil, resErr } - } else { + } + // If dynamicThumbnails is true but there are too many thumbnails being actively generated, we can fall back + // to trying to use a pre-generated thumbnail + if thumbnail == nil { thumbnails, err := db.GetThumbnails(r.MediaMetadata.MediaID, r.MediaMetadata.Origin) if err != nil { r.Logger.WithError(err).Error("Error looking up thumbnails") @@ -305,13 +308,15 @@ func (r *downloadRequest) getThumbnailFile(filePath types.Path, activeThumbnailG // If we get a thumbnail, we're done. var thumbnailSize *types.ThumbnailSize thumbnail, thumbnailSize = thumbnailer.SelectThumbnail(r.ThumbnailSize, thumbnails, thumbnailSizes) - if thumbnailSize != nil { + // If dynamicThumbnails is true and we are not over-loaded then we would have generated what was requested above. + // So we don't try to generate a pre-generated thumbnail here. + if thumbnailSize != nil && dynamicThumbnails == false { r.Logger.WithFields(log.Fields{ "Width": thumbnailSize.Width, "Height": thumbnailSize.Height, "ResizeMethod": thumbnailSize.ResizeMethod, }).Info("Pre-generating thumbnail for immediate response.") - thumbnail, resErr = r.generateThumbnail(filePath, *thumbnailSize, activeThumbnailGeneration, db) + thumbnail, resErr = r.generateThumbnail(filePath, *thumbnailSize, activeThumbnailGeneration, maxThumbnailGenerators, db) if resErr != nil { return nil, nil, resErr } @@ -348,18 +353,21 @@ func (r *downloadRequest) getThumbnailFile(filePath types.Path, activeThumbnailG return thumbFile, thumbnail, nil } -func (r *downloadRequest) generateThumbnail(filePath types.Path, thumbnailSize types.ThumbnailSize, activeThumbnailGeneration *types.ActiveThumbnailGeneration, db *storage.Database) (*types.ThumbnailMetadata, *util.JSONResponse) { +func (r *downloadRequest) generateThumbnail(filePath types.Path, thumbnailSize types.ThumbnailSize, activeThumbnailGeneration *types.ActiveThumbnailGeneration, maxThumbnailGenerators int, db *storage.Database) (*types.ThumbnailMetadata, *util.JSONResponse) { logger := r.Logger.WithFields(log.Fields{ "Width": thumbnailSize.Width, "Height": thumbnailSize.Height, "ResizeMethod": thumbnailSize.ResizeMethod, }) - var err error - if err = thumbnailer.GenerateThumbnail(filePath, thumbnailSize, r.MediaMetadata, activeThumbnailGeneration, db, logger); err != nil { + busy, err := thumbnailer.GenerateThumbnail(filePath, thumbnailSize, r.MediaMetadata, activeThumbnailGeneration, maxThumbnailGenerators, db, r.Logger) + if err != nil { logger.WithError(err).Error("Error creating thumbnail") resErr := jsonerror.InternalServerError() return nil, &resErr } + if busy { + return nil, nil + } var thumbnail *types.ThumbnailMetadata thumbnail, err = db.GetThumbnail(r.MediaMetadata.MediaID, r.MediaMetadata.Origin, thumbnailSize.Width, thumbnailSize.Height, thumbnailSize.ResizeMethod) if err != nil { @@ -406,7 +414,7 @@ func (r *downloadRequest) getRemoteFile(cfg *config.MediaAPI, db *storage.Databa if mediaMetadata == nil { // If we do not have a record, we need to fetch the remote file first and then respond from the local file - resErr := r.fetchRemoteFileAndStoreMetadata(cfg.AbsBasePath, *cfg.MaxFileSizeBytes, db, cfg.ThumbnailSizes, activeThumbnailGeneration) + resErr := r.fetchRemoteFileAndStoreMetadata(cfg.AbsBasePath, *cfg.MaxFileSizeBytes, db, cfg.ThumbnailSizes, activeThumbnailGeneration, cfg.MaxThumbnailGenerators) if resErr != nil { return resErr } @@ -468,7 +476,7 @@ func (r *downloadRequest) broadcastMediaMetadata(activeRemoteRequests *types.Act } // fetchRemoteFileAndStoreMetadata fetches the file from the remote server and stores its metadata in the database -func (r *downloadRequest) fetchRemoteFileAndStoreMetadata(absBasePath types.Path, maxFileSizeBytes types.FileSizeBytes, db *storage.Database, thumbnailSizes []types.ThumbnailSize, activeThumbnailGeneration *types.ActiveThumbnailGeneration) *util.JSONResponse { +func (r *downloadRequest) fetchRemoteFileAndStoreMetadata(absBasePath types.Path, maxFileSizeBytes types.FileSizeBytes, db *storage.Database, thumbnailSizes []types.ThumbnailSize, activeThumbnailGeneration *types.ActiveThumbnailGeneration, maxThumbnailGenerators int) *util.JSONResponse { finalPath, duplicate, resErr := r.fetchRemoteFile(absBasePath, maxFileSizeBytes) if resErr != nil { return resErr @@ -497,10 +505,13 @@ func (r *downloadRequest) fetchRemoteFileAndStoreMetadata(absBasePath types.Path } go func() { - err := thumbnailer.GenerateThumbnails(finalPath, thumbnailSizes, r.MediaMetadata, activeThumbnailGeneration, db, r.Logger) + busy, err := thumbnailer.GenerateThumbnails(finalPath, thumbnailSizes, r.MediaMetadata, activeThumbnailGeneration, maxThumbnailGenerators, db, r.Logger) if err != nil { r.Logger.WithError(err).Warn("Error generating thumbnails") } + if busy { + r.Logger.Warn("Maximum number of active thumbnail generators reached. Skipping pre-generation.") + } }() r.Logger.WithFields(log.Fields{ diff --git a/src/github.com/matrix-org/dendrite/mediaapi/writers/upload.go b/src/github.com/matrix-org/dendrite/mediaapi/writers/upload.go index 5ce708b8a..7071b4179 100644 --- a/src/github.com/matrix-org/dendrite/mediaapi/writers/upload.go +++ b/src/github.com/matrix-org/dendrite/mediaapi/writers/upload.go @@ -152,7 +152,7 @@ func (r *uploadRequest) doUpload(reqReader io.Reader, cfg *config.MediaAPI, db * } } - if resErr := r.storeFileAndMetadata(tmpDir, cfg.AbsBasePath, db, cfg.ThumbnailSizes, activeThumbnailGeneration); resErr != nil { + if resErr := r.storeFileAndMetadata(tmpDir, cfg.AbsBasePath, db, cfg.ThumbnailSizes, activeThumbnailGeneration, cfg.MaxThumbnailGenerators); resErr != nil { return resErr } @@ -215,7 +215,7 @@ func (r *uploadRequest) Validate(maxFileSizeBytes types.FileSizeBytes) *util.JSO // The order of operations is important as it avoids metadata entering the database before the file // is ready, and if we fail to move the file, it never gets added to the database. // Returns a util.JSONResponse error and cleans up directories in case of error. -func (r *uploadRequest) storeFileAndMetadata(tmpDir types.Path, absBasePath types.Path, db *storage.Database, thumbnailSizes []types.ThumbnailSize, activeThumbnailGeneration *types.ActiveThumbnailGeneration) *util.JSONResponse { +func (r *uploadRequest) storeFileAndMetadata(tmpDir types.Path, absBasePath types.Path, db *storage.Database, thumbnailSizes []types.ThumbnailSize, activeThumbnailGeneration *types.ActiveThumbnailGeneration, maxThumbnailGenerators int) *util.JSONResponse { finalPath, duplicate, err := fileutils.MoveFileWithHashCheck(tmpDir, r.MediaMetadata, absBasePath, r.Logger) if err != nil { r.Logger.WithError(err).Error("Failed to move file.") @@ -243,10 +243,13 @@ func (r *uploadRequest) storeFileAndMetadata(tmpDir types.Path, absBasePath type } go func() { - err := thumbnailer.GenerateThumbnails(finalPath, thumbnailSizes, r.MediaMetadata, activeThumbnailGeneration, db, r.Logger) + busy, err := thumbnailer.GenerateThumbnails(finalPath, thumbnailSizes, r.MediaMetadata, activeThumbnailGeneration, maxThumbnailGenerators, db, r.Logger) if err != nil { r.Logger.WithError(err).Warn("Error generating thumbnails") } + if busy { + r.Logger.Warn("Maximum number of active thumbnail generators reached. Skipping pre-generation.") + } }() return nil