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.

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

Using go-sitemap-generator changes behavior of entire program #86

Closed
SamWhited opened this issue Mar 26, 2019 · 2 comments
Closed

Using go-sitemap-generator changes behavior of entire program #86

SamWhited opened this issue Mar 26, 2019 · 2 comments
Assignees

Comments

@SamWhited
Copy link

@SamWhited SamWhited commented Mar 26, 2019

Describe the bug

The package ikeikeikeike/go-sitemap-generator currently sets the value of GOMAXPROCs (to NumCPUs) every time you create a new sitemap.
In newer versions v2 they let you pass through the value you want, but this doesn't make this behavior any less insane.
This will limit the number of OS threads that can execute Go code for the entire program, likely causing unexpected behavior.
It will also happen whenever you create a new sitemap; if you do this more than once, you will have to set it back to whatever you prefer each time.
Generally speaking, if you don't absolutely need to make changes, it's best to just leave the runtime alone.

It also performs other sorts of insane behavior like logging to the global logger, bypassing application level logging conventions and causing unexpected log messages that you can't easily turn off..

See: https://github.com/ikeikeikeike/go-sitemap-generator/blob/v1/stm/sitemap.go#L11

Please consider removing the go-sitemap-generator package as a dependency; I don't know if there's anything better out there, but this behavior is bizarre and a bit scary.

I'm trying to figure out why they did this and may file an issue upstream as well, but given that it's now baked into their API as of version 2 I don't know that it's worth trying to salvage the package. Probably best to avoid it entirely.

@thebaer
Copy link
Member

@thebaer thebaer commented Mar 26, 2019

Thanks for the report -- good catch. Based on a quick look at their README, I'd guess this odd behavior is so you can potentially disable concurrency. Though it's not clear why you might do that.

To fix this we'll need to fork the library or find an alternative. A quick search turned this up: https://github.com/snabb/sitemap -- not sure if it's a good replacement, but anyone should feel free to investigate.

@SamWhited
Copy link
Author

@SamWhited SamWhited commented Mar 27, 2019

I'd guess this odd behavior is so you can potentially disable concurrency

Yah, I really don't get that. You could always just do that yourself before calling any of the methods of the library; why would the library have anything to do with concurrency itself? The library doing it just forces you into a corner. Oh well. shrug

A quick search turned this up: https://github.com/snabb/sitemap -- not sure if it's a good replacement

A quick glance at the godoc makes me much more confident that they actually know and are writing good Go, but I haven't looked at the code itself. It is worth noting that it also has a dependency on github.com/snabb/diagio which is unfortunate. Something this simple really doesn't need to pull in transient dependencies.

@thebaer thebaer self-assigned this Jul 19, 2019
@thebaer thebaer removed the help wanted label Jul 19, 2019
thebaer added a commit that referenced this issue Jul 20, 2019
This upgrades the library to v2, which lets you specify that GOMAXPROCs
should always be the max number of CPUs -- and then sets it to that.

One side effect of this is that images are no longer listed in sitemaps.
I'm somehow at a loss on how to build and append the images array we
need, with the library's latest changes.

Fixes #86
@thebaer thebaer added the performance label Aug 1, 2019
@thebaer thebaer closed this in #141 Aug 2, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

2 participants
You can’t perform that action at this time.