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

allow configurable timeout for parallel rendering #191

Closed
fivetanley opened this issue Jul 27, 2018 · 6 comments
Closed

allow configurable timeout for parallel rendering #191

fivetanley opened this issue Jul 27, 2018 · 6 comments
Assignees
Labels

Comments

@fivetanley
Copy link

Sometimes, one has a large amount of markdown parsing to do and the default timeout of Task.await (5000ms) is not long enough to do it. It would be great to be able to configure the timeout passed to Task.await instead of overriding pmap, which is technically private.

I would be happy to PR this given some direction and approval that this is something the project wants.

Thanks for making/maintaining Earmark! It's helped us a lot.

@RobertDober
Copy link
Collaborator

RobertDober commented Jul 29, 2018

Yes indeed that is something we want, well need, if you have hit that hard limit.

I am certainly interested in a PR, the idea would be to instrument Task.awaitwith a timeout from Earmark.Options which should also be implemented as a CLI argument for the escript.

That said I am a little bit intrigued where you hit that limit. In the line scanner (https://github.com/pragdave/earmark/blob/master/lib/earmark/line.ex#L81) or in the HTML renderer (https://github.com/pragdave/earmark/blob/master/lib/earmark/html_renderer.ex#L16), I'd guess in the later, man (pun intended).
Nevertheless the amount of markdown you process must be impressive.

If you do not call Earmark from the command line, but as a lib, a workaround for the time being, could be to set Enum.map as the mapper in Earmark.Options:

Thank you for your kind words, but it is I who is thankful to work on a useful open source project with kind, bright and helpful people around, and the privilege to work with Dave Thomas.

@RobertDober
Copy link
Collaborator

Oh sorry I missed that you were aware of the workaround, bad communication skills 😢

@RobertDober RobertDober self-assigned this Aug 4, 2018
@RobertDober RobertDober added this to the 1.2.6 milestone Aug 4, 2018
@RobertDober
Copy link
Collaborator

RobertDober commented Aug 4, 2018

I am afraid that my implementation of this feature, which was simple, beware of simple solutions, right 🤕 ...

ran into a regression issue.

The failing test in the PR demonstrates it quite clearly

https://github.com/pragdave/earmark/pull/192/files#diff-ff32feea7286dc7cc37031859c6af5a3R13

I think you need to override %Options{mapper:...} after all, as it is, technically speaking public and we might run into the regression above.

Unless I can find an elegant solution to this dilemma I might need to close this issue, but I'll give it some time and will definitely listen to suggestions.

@pragdave your guidence would be appreciated.

@RobertDober
Copy link
Collaborator

RobertDober commented Aug 4, 2018

here is something which might work (far from elegant though)

#193

I feel however that it is quite kludgy and really would like @pragdave 's opinion on this.

@fivetanley
Copy link
Author

@RobertDober thanks for looking into this! we ended overriding the mapper for now, and it's good to have a maintainer's blessing on doing so if it's technically public. :)

@RobertDober
Copy link
Collaborator

please let me know if it is enough, because Line.scan_lines uses Earmark.pmap/2 with the 5000ms hardcoded timeout, I will definitely change this, but being quite busy this might need some time to be done.

So you actually hit a bug that I had not even seen in the first analysis of the issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants