Comments 20
Спасибо за статью!
Спасибо за статью, очень нравитcя как вы рассуждаете над реализацией того или иного момента в рограмме и объясняете, почему лучше сделать так то, хотя есь есть варианты такие то. Круто.
По поводу оптимизации, можно встоить в структуру trudVsem еще одно поле - Url типа *url.URL и спарсить туда адрес единажды в функции init (конструктор) чтобы не делать это при каждом запросе. Правда в данном контексте это мало что даст для производительности, но с точки зрения оптимизации может это хорошо? :)
Ну да, я об этом написал вот тут (может вышло не очень понятно):
Единственное, что можно было бы сделать — вынести парсинг константы отдельно, но этот участок кода настолько нетребователен к производительности, что разделять эту логику и выносить куда-то ее часть, я считаю нецелесообразным.
Но смысл в том, что мы отделяем маленькую часть от общей логики (создание URL реквеста) и тем самым ухудшаем чтение. Когда вам постоянно приходится бегать по коду глазами вверх-вниз, чтобы собрать в голове воедино кусочки чего то общего - это не очень правильно. А т.к. эта логика вообще не влияет на производительность, я не стал ее делить.
Может не выносить и воспользоваться sync.Once?
Хороший примитив для реализации конкурентности в случае, если нам нужно что-то выполнить единожды, но тут совсем не то.
Попробуйте показать вариант реализации этой функции с использованием sync.Once без разделения логики. И при этом не забывайте о "состоянии гонки". Нам все равно при этом не обойтись без дополнительных полей + наверняка придется использовать еще и мьютексы.
Дык просто выносите url в поле структуры и оборачиваете парсинг url в once.Do(). Гарантий которые предоставляет once достаточно для синхронизации.
Если once.Do вы оставите внутри рассматриваемой функции, тогда это будет работать только при условии, что запуск функции в разных рутинах не пересечется
А этого нельзя гарантировать полностью даже сейчас, не говоря уже о какой то долгосрочной перспективе
Если вы напишете код, я напишу вариант воспроизведения шагов race condition или есть вариант, что я вас неправильно понял.
var u *url.URL
var o = &sync.Once{}
func main() {
o.Do(func() {
u, _ = url.ParseRequestURI("http://google.com")
})
print(u.String())
}Если once.Do вы оставите внутри рассматриваемой функции, тогда это будет работать только при условии, что запуск функции в разных рутинах не пересечется
Нет это будет работать безопасно, я в посте дал ссылку на memory model
вот этот код как раз будет работать безопасно, но он не выполняет функций, которые выполняет код моей функции. Вы теряете контекст, а поэтому тут не видно о чем я говорю. мы вобще пока общаемся в разных контекстах, но давайте я проясню...
func (tv *trudVsem) newVacanciesRequest(ctx context.Context, text string, offset, limit int) (*http.Request, error) {
o.Do(func() {
tv.URL, err := url.ParseRequestURI(serviceURL)
if err != nil {
// do something
}
})
query := url.Values{
"text": []string{text},
"offset": []string{strconv.Itoa(offset)},
"limit": []string{strconv.Itoa(limit)},
"modifiedFrom": []string{time.Now().Add(-time.Hour * 168).UTC().Format(time.RFC3339)},
}
tv.URL.RawQuery = query.Encode()
return http.NewRequestWithContext(ctx, http.MethodGet, tv.URL.String(), http.NoBody)
}Если вы такой код имели в виду, то вот оно работает не потокобезопасно. Если не такой - покажите свой вариант
Ой, нет здесь я ошибся. это работать будет. И будет работать правильно. но есть одно но. Сначала ответьте правильно ли я понял предлагаемый код
Все верно, это safe код, а в чем но?
Вам придется продумать вариант обработки ошибки err. Даже не смотря на то, что она маловероятна. Есть риск развития такой логики до такого состояния, что вам придется делать много сложных вещей внутри этого Do и это будет усложнять логику и читаемость экспоненциально. А какой Профит? Экономия 1 миллисекунды каждые 5-10 минут против простоты кода проигрывает на любых дистанциях.
Гораздо проще парсинг вынести в инит, но об этом я сам уже написал.
Ошибка здесь не просто маловероятна. Учитывая что мы парсим константу идемпотентной операцией - не проверять тут ошибку вполне нормально.
По поводу много сложный вещей внутри Do - да он не для этого, но и я предложил использовать его только в конкретном кейсе инициализации.
Ну ок, я не говорю, что этот вариант плохой. Он хорош. И да, я согласен, что можно не очень дотошно обрабатывать ошибку парсинга константы или игнорить вообще. Но я довольно консервативен в том, что касается читабельности и поддерживаемости кода. Поэтому я не люблю такие вещи как ORM или Query Builder (ни в коем случае не хочу уводить от темы или накидывать на вентилятор).
Но хочу прокомментировать ваш вариант использования sync.Once в конкретном кейсе инициализации. И вот как я рассуждаю:
Представим себе возможные варианты развития событий
Какой то мейнтейнер Вася добавляет новый функционал и по какой то причине он изменил ссылку в константе (может нашел апи получше). Вот он сидит и не понимает почему у него ничего не работает и чего не так с урлом (просто он много накодил и не понимает куда смотреть в первую очередь). Окей, его это не сильно заморочит (в конце концов он найдет причину), но все-таки причина не выпадет в лог, выпадет в лог только следствие.
Следующий мейнтейнер Петя чего-то добавляет и замечает sync.Once и думает, как же это удобно, можно сделать его функцию производительнее. А еще там ошибка не логируется (или логируется, но это уже будет бойлерпринт). Добавляет он свое туда и ошибки не обрабатывает тоже. А мейнтейнер Вася из первого кейса в следующий раз будет дольше страдать.
И еще один умный чел Дмитрий (будь как Дмитрий). Он добавляет в функцию возможность делать URL для разных эндпоинтов (ну просто переиспользует код). Видит, что там sync.Once и одно поле для хранения распаршеного урла, а ему нужно два. Он убирает нафиг все наши sync.Once и делает свое дело, а потом думает, ну пусть это работает на 1 милисекунду дольше, зато теперь читаемость лучше и никаких граблей. После этого Васе станет легче жить.
Учитывая все это я думаю, что sync.Once в данном случае хуже, чем вынести парсинг урла в Init и не париться. Хотя ни то ни другое не является плохим вариантом само по себе. И то и другое варианты подходящие, но лично мое мнение вот в этом комменте выше.
А вообще мы, наверно, можем использовать замыкание вот так:
func (tv *trudVsem) newVacanciesRequest(ctx context.Context, text string, offset, limit int) (*http.Request, error) {
var err error
o.Do(func() {
tv.URL, err = url.ParseRequestURI(serviceURL)
})
if err != nil {
// вот тут обработка
}должно работать ИМХО
Стоп стоп, вы хотите лечить симптом или болезнь?
Я говорил о лечении симптома - просто добавили sync.Once - победили лишний парсинг каждый раз на реквест. Не более.
Если мы говорим о лечении болезни то надо изначально сделать значение url'a конфигурируемым и добиться того чтобы в структуру trudVsem прокидывался *url.URL на этапе создания этой структуры (если хотите на этапе инициализации). А из строки в url.URL мы преобразовываем на этапе чтения конфигурации.
То чувство, когда поторопился в споре и зафейлился =)
А ведь подождал бы с отправкой 1 минуту и понял бы свою ошибку
del
Спасибо за статью! Ждем продолжения!
Пишем сервис на GO. Backend для апплета