search

mozilla / apk-factory-service

Web service which converts Open Web Apps into native Android apps
41 stars 18 forks source link

possible fix for Bug#1060473 #82

Closed dannycoates closed 9 years ago

dannycoates commented 9 years ago

Under still unknown conditions HttpFileLoader.copy fails to call the callback while loading icons for the apk_project_builder. This causes the build to hang and never decrement the activeBuilds counter which eventually exhausts the concurrent build limit.

This change buffers the icon instead of streaming to make the error handling more explicit.

dannycoates commented 9 years ago

@kumar303 The timestamps on the concurrent build log line were invaluable in tracking this down :+1:

andymckay commented 9 years ago

Trying to find someone to r? this for you. Are there any unit tests on this code? Should we add one?

dannycoates commented 9 years ago

Are there any unit tests on this code? Should we add one?

It could definitely use some unit test love. I haven't yet been able to reproduce the conditions that caused the original code to fail but I'm still trying.

Unfortunately this is a speculative fix based on the logs but I didn't want to delay this very likely fix while searching for the root cause. When I do find it I will certainly add a test.

andymckay commented 9 years ago

Fair enough on the testing.

andymckay commented 9 years ago

i haven't found anyone to r? this, maybe merge then we can test on stage?

dannycoates commented 9 years ago

Maybe @chilts @seanmonstar or @zaach could r that this won't blow up?

maybe merge then we can test on stage?

I don't have merge rights on this repo so it can't be me

seanmonstar commented 9 years ago

Looks fine to me, assuming the apk is never so huge as to kill the Node process. r+