Skip to content

Conversation

craigfe
Copy link
Member

@craigfe craigfe commented Jan 29, 2025

While downstreaming a new slog feature to Monzo's internal monorepo, I discovered that the empty typhon.Request value panics when the various context.Context methods are called on it:

typhon.Request{}.Value("foo") // panics

This happens because typhon.Request embeds context.Context, and so the zero value of typhon.Request has a typed nil context. This PR adds wrappers for the context.Context methods that behave like context.Background() on the empty typhon.Request value instead.

This is arguably not a bug: users of the library are expected to use either typhon.NewRequest or typhon.NewRawRequest to construct request values, these functions explicitly handle the nil context case, and the library makes no promises about the validity of the zero value.

Either way, we already have unit tests in our internal monorepo that build typhon.Request values manually and with the current typhon.Request implementation those tests are unstable: introducing a new call to one of the context.Context methods in a library (as I did in monzo/slog#22), can indirectly cause consumers of that library to panic. It's not difficult to add wrapper methods that work correctly for the empty typhon.Request value, so that's exactly what this PR does.

@craigfe craigfe marked this pull request as ready for review January 29, 2025 09:58
@craigfe craigfe requested a review from EddShaw January 30, 2025 08:54
strategy:
matrix:
go-version: [1.17.x, 1.18.x]
go-version: [1.19.x, 1.23.x]
Copy link
Member Author

Choose a reason for hiding this comment

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

Fly-by change: the minimum supported version in go.mod is already 1.19, and I've introduced a use of any that causes this library to fail to compile on 1.17.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think there may be references to 1.17 and 1.18 elsewhere, breaking CI 🙈

Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm, no. Not sure why CI is blocked on actions that no longer exist. 🤔

Copy link
Contributor

@EddShaw EddShaw left a comment

Choose a reason for hiding this comment

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

Nice!

@EddShaw
Copy link
Contributor

EddShaw commented Jan 30, 2025

I think we might need someone with repo permissions to fix branch protection rules.

@craigfe craigfe merged commit 8b5d72a into monzo:master Jan 31, 2025
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants