Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Misc fixes #560

Open
wants to merge 22 commits into
base: main
Choose a base branch
from
Open

Misc fixes #560

Changes from 1 commit
Commits
Show all changes
22 commits
Select commit Hold shift + click to select a range
9498f39
remove util.ArrayContains
mpldr Feb 26, 2024
8b0a8e6
use format strings for string building
mpldr Feb 26, 2024
6dd81b1
fix send on closed channel
mpldr Feb 27, 2024
3e03f07
fix panic being used as a way to communicate errors
mpldr Feb 27, 2024
ef35ac7
get rid of some unnecessary panics
mpldr Feb 27, 2024
9518366
improve variable naming
mpldr Feb 27, 2024
87964bb
introduce the concept of time into the codebase
mpldr Mar 2, 2024
186f1c9
rename thumbnailing/i to thumbnailing/preview
mpldr Mar 2, 2024
c518070
drop model package
mpldr Mar 2, 2024
cc20a34
minor preview metadata backend refactoring
mpldr Mar 2, 2024
f81662d
inline GetID3Tags
mpldr Mar 2, 2024
682b41f
move thumbnailing/u to thumbnailing/preview/metadata
mpldr Mar 2, 2024
edd19b2
move pipelines/_steps to pipelines/steps
mpldr Mar 3, 2024
ca68585
move cmd/utilities/_common to cmd/utilities/common
mpldr Mar 3, 2024
fadeb9a
move api/_apimeta to api/apimeta
mpldr Mar 3, 2024
8515e0b
move api/_responses to api/responses
mpldr Mar 3, 2024
a6b6598
move api/_routers to api/routers
mpldr Mar 3, 2024
6fc49c2
move api/_auth_cache to api/auth_cache
mpldr Mar 3, 2024
dc277e9
move api/_debug to api/debug
mpldr Mar 3, 2024
1f3380e
move cmd/homeserver_live_importers/_common to cmd/homeserver_live_imp…
mpldr Mar 3, 2024
a31465b
move cmd/homeserver_offline_importers/_common to cmd/homeserver_offli…
mpldr Mar 3, 2024
0c957d8
move cmd/homeserver_offline_exporters/_common to cmd/homeserver_offli…
mpldr Mar 3, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 3 additions & 8 deletions pipelines/_steps/url_preview/upload_image.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,21 +21,16 @@ func UploadImage(ctx rcontext.RequestContext, image *m.PreviewImage, onHost stri
defer image.Data.Close()
pr, pw := io.Pipe()
tee := io.TeeReader(image.Data, pw)
mediaChan := make(chan *database.DbMedia)
defer close(mediaChan)
mediaChan := make(chan *database.DbMedia, 1)
go func() {
media, err := pipeline_upload.Execute(ctx, onHost, "", io.NopCloser(tee), image.ContentType, image.Filename, userId, datastores.LocalMediaKind)
if err != nil {
_ = pw.CloseWithError(err)
} else {
_ = pw.Close()
}
go func() {
defer func() {
recover() // consume write-to-closed-channel errors
}()
mediaChan <- media
}()
mediaChan <- media
close(mediaChan)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it's a bit uncomfortable to me to have the close in the goroutine when the channel is created externally - please restore the defer close(mediaChan) to avoid potential resource leaks.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, this is a bug. Recovering from a panic does not remedy unclear channel ownership. The go routine for writing is no longer necessary since this is a buffered channel now. And the channel can just be closed afterwards. A simple look at the possible code paths confirms that there is no way the channel could remain unclosed.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The panic recovery is indeed a bug, though the defer close(mediaChan) needs to be moved up near the channel creation please. The remainder of the change is fine, I think.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No. This is a concurrency bug. The channel closing can be deferred in the writing function, but never outside of it. Otherwise, you end with a potential write on a closed channel. And since uploading likely takes a few milliseconds more than thumbnailing (at least on reasonably fast hardware), this will happen more often than not. So the best I could do is:

go func(){
    defer close(mediachan)
    // upload and stuff

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should be blocking on a read from the channel before the defer would fire - if we're not, then the concurrency is broken for sure. I'm not really comfortable using a different close style in a single section of the code, and would rather go out of the way to make defer close() immediately after creation work reliably.

}()

w := 0
Expand Down