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

trails:do_store hangs for 60 seconds #82

Closed
tothlac opened this issue Jan 4, 2018 · 4 comments
Closed

trails:do_store hangs for 60 seconds #82

tothlac opened this issue Jan 4, 2018 · 4 comments

Comments

@tothlac
Copy link

tothlac commented Jan 4, 2018

Executing this line takes sometimes a minute. The store function is called and soon after that remove is also called. It does not take always one minute but in a lot of cases I can see execution is stopped there. When I comment this line out everything works well.

Actually the list in the second parameter of do_store/2 can be huge. Is there a reason why application:ensure_all_started is called million times? Would not it be enough to start those applications only when trails starts?

{ok, _} = application:ensure_all_started(trails),

@elbrujohalcon
Copy link
Member

That line is there just as a convenience function so you don't need to manually ensure trails is started unless you use trails:store/1,2. I'm not sure why it shouldn't return instantly if trails is already started 🤔

@elbrujohalcon
Copy link
Member

In any case, as long as it passes all tests, I'm inclined to accept a PR removing that line

tothlac pushed a commit to tothlac/cowboy-trails that referenced this issue Jan 5, 2018
Unfortunately with this change store can't be called anymore without
starting up trails earlier.
@tothlac
Copy link
Author

tothlac commented Jan 5, 2018

I've created a PR for this. Of course I had to change some lines in trails_SUITE because at some points it expects trails is not started before it calls trails:store. CT && dialyzer passes. Unfortunately cover was not 100% after the change, but it was also the same locally on your master. Do you expect cover to reach 100%?

@elbrujohalcon
Copy link
Member

That would be lovely, but since it wasn't 100% already… the best we can do is open a different ticket for that (#84).

elbrujohalcon added a commit that referenced this issue Jan 5, 2018
[#82] Don't call ensure_all_started from do_store
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

No branches or pull requests

2 participants