Misc fixes #560

mpldr · 2024-02-26T15:07:59Z

Just some minor things I've noticed while perusing the codebase. This includes:

Since Go 1.21, there is a slices package in the standard library, that implements the same functionality more comprehensively. Replace the uses of ArrayContains with slices.Contains.

mpldr · 2024-03-02T17:12:07Z

okay, now for the scariest part…

…orters/internal

…ne_importers/internal

…ne_exporters/internal

mpldr · 2024-03-03T14:35:29Z

A number of these can probably be added to the .git-blame-ignore-revs file

turt2live

Thanks for taking the time to submit these changes! A lot of them are agreeable and certainly improve the code quality, though several don't appear to consider why a particular decision was made.

I've left comments by going through the commits individually. For comments which affect the whole commit, I've arbitrarily left the comment on the first file so there's some visible threading in the github UI. I suggest reviewing this review by going through the commits individually.

It also appears as though some commits include unrelated changes, making it hard to accept this commit by commit. I would suggest making the changes recommended in this review and opening a dedicated PR for each task instead, encouraging commits to be distinct and limited in scope. This will also help with the eventual merge strategy, as MMR's policy is to squash merge rather than cherry pick or create a merge commit.

If you have any questions about the feedback here, please feel free to ask either here or in the support room.

turt2live · 2024-03-14T04:31:12Z

api/_routers/00-install-params.go


+// var NumericIdRegex = regexp.MustCompile("[0-9]+")


looks like this got moved down incorrectly

turt2live · 2024-03-14T04:31:47Z

api/_responses/errors.go

@@ -48,8 +52,8 @@ func GuestAuthFailed() *ErrorResponse {
 	return &ErrorResponse{common.ErrCodeNoGuests, "Guests cannot use this endpoint", common.ErrCodeNoGuests}
 }

-func BadRequest(message string) *ErrorResponse {
-	return &ErrorResponse{common.ErrCodeUnknown, message, common.ErrCodeBadRequest}
+func BadRequest(err error) *ErrorResponse {


These functions deliberately do not take error objects to avoid accidentally leaking internal details to callers - please revert this change.

there's an exception to this rule later on in the federation test/info endpoints, as those endpoints are primarily used for debugging or by admins rather than random consumers.

turt2live · 2024-03-14T04:34:35Z

database/db.go

@@ -70,59 +70,59 @@ func openDatabase(connectionString string, maxConns int, maxIdleConns int) error
 	var err error

 	if d.conn, err = sql.Open("postgres", connectionString); err != nil {
-		return errors.New("error connecting to db: " + err.Error())
+		return fmt.Errorf("error connecting to db: %w", err)


we should probably be using errors.Join() in these cases to avoid losing context on the stack.

%w is a special formatting instruction for fmt.Errorf() that wraps the error.

hmm, that feels a bit magic. errors.Join is more clear.

turt2live · 2024-03-14T04:35:44Z

matrix/server_discovery.go

-var apiUrlCacheInstance *cache.Cache
-var apiUrlSingletonLock = &sync.Once{}
+var (
+	apiUrlCacheInstance *cache.Cache
+	apiUrlSingletonLock = &sync.Once{}
+)


please undo this change

turt2live · 2024-03-14T04:38:50Z

pipelines/_steps/url_preview/upload_image.go

-			mediaChan <- media
-		}()
+		mediaChan <- media
+		close(mediaChan)


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.

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.

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.

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

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.

turt2live · 2024-03-14T04:57:07Z

api/auth_cache/auth_cache.go

please revert 6fc49c2 - the package naming is deliberate

turt2live · 2024-03-14T04:57:21Z

api/debug/pprof.go

please revert dc277e9 - the package naming is deliberate

turt2live · 2024-03-14T04:57:39Z

cmd/homeserver_live_importers/import_dendrite/main.go

please revert 1f3380e - the package naming is deliberate

turt2live · 2024-03-14T04:57:55Z

cmd/homeserver_offline_importers/export_dendrite_for_import/main.go

please revert a31465b - the package naming is deliberate

turt2live · 2024-03-14T04:58:09Z

cmd/homeserver_offline_exporters/import_to_synapse/main.go

please revert 0c957d8 - the package naming is deliberate

remove util.ArrayContains

9498f39

Since Go 1.21, there is a slices package in the standard library, that implements the same functionality more comprehensively. Replace the uses of ArrayContains with slices.Contains.

mpldr force-pushed the misc-fixes branch 2 times, most recently from 576cf4b to 4044eb6 Compare February 27, 2024 19:52

mpldr added 2 commits March 2, 2024 18:10

use format strings for string building

8b0a8e6

fix send on closed channel

6dd81b1

mpldr force-pushed the misc-fixes branch from 4044eb6 to 93bc7c9 Compare March 2, 2024 17:11

fix panic being used as a way to communicate errors

3e03f07

mpldr force-pushed the misc-fixes branch 3 times, most recently from d45c407 to 07ca3f2 Compare March 3, 2024 13:46

mpldr added 18 commits March 3, 2024 15:20

get rid of some unnecessary panics

ef35ac7

improve variable naming

9518366

introduce the concept of time into the codebase

87964bb

rename thumbnailing/i to thumbnailing/preview

186f1c9

drop model package

c518070

minor preview metadata backend refactoring

cc20a34

inline GetID3Tags

f81662d

move thumbnailing/u to thumbnailing/preview/metadata

682b41f

move pipelines/_steps to pipelines/steps

edd19b2

move cmd/utilities/_common to cmd/utilities/common

ca68585

move api/_apimeta to api/apimeta

fadeb9a

move api/_responses to api/responses

8515e0b

move api/_routers to api/routers

a6b6598

move api/_auth_cache to api/auth_cache

6fc49c2

move api/_debug to api/debug

dc277e9

move cmd/homeserver_live_importers/_common to cmd/homeserver_live_imp…

1f3380e

…orters/internal

move cmd/homeserver_offline_importers/_common to cmd/homeserver_offli…

a31465b

…ne_importers/internal

move cmd/homeserver_offline_exporters/_common to cmd/homeserver_offli…

0c957d8

…ne_exporters/internal

mpldr force-pushed the misc-fixes branch from 648be6c to 0c957d8 Compare March 3, 2024 14:33

mpldr marked this pull request as ready for review March 3, 2024 14:33

mpldr changed the title ~~WIP: Misc fixes~~ Misc fixes Mar 3, 2024

turt2live self-requested a review March 4, 2024 18:52

turt2live requested changes Mar 14, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Misc fixes #560

Misc fixes #560

mpldr commented Feb 26, 2024 •

edited

Loading

mpldr commented Mar 2, 2024

mpldr commented Mar 3, 2024 •

edited

Loading

turt2live left a comment

turt2live Mar 14, 2024

turt2live Mar 14, 2024

turt2live Mar 14, 2024

turt2live Mar 14, 2024

mpldr Mar 14, 2024

turt2live Mar 14, 2024

turt2live Mar 14, 2024

turt2live Mar 14, 2024

mpldr Mar 14, 2024

turt2live Mar 14, 2024

mpldr Mar 14, 2024

turt2live Mar 14, 2024

turt2live Mar 14, 2024

turt2live Mar 14, 2024

turt2live Mar 14, 2024

turt2live Mar 14, 2024

turt2live Mar 14, 2024

Misc fixes #560

Are you sure you want to change the base?

Misc fixes #560

Conversation

mpldr commented Feb 26, 2024 • edited Loading

mpldr commented Mar 2, 2024

mpldr commented Mar 3, 2024 • edited Loading

turt2live left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mpldr commented Feb 26, 2024 •

edited

Loading

mpldr commented Mar 3, 2024 •

edited

Loading