search

porkbuns / shmile

The DIY photobooth, at your service.
75 stars 39 forks source link

When the browser refreshes, multiple copies of images are added to the image state causing a server crash #34

Closed bsalinas closed 9 years ago

bsalinas commented 9 years ago

I ran across a bug where sometimes the server would crash because the image_src_list was getting extra copies of images.

This resulted in an error message like the following:

img_src_list is: public/temp/test_photo.jpg,public/temp/test_photo.jpg,public/temp/test_photo.jpg,public/temp/test_photo.jpg,public/temp/test_photo.jpg,public/temp/test_photo.jpg,public/temp/test_photo.jpg,public/temp/test_photo.jpg,public/temp/test_photo.jpg,public/temp/test_photo.jpg,public/temp/test_photo.jpg,public/temp/test_photo.jpg,public/temp/test_photo.jpg,public/temp/test_photo.jpg,public/temp/test_photo.jpg,public/temp/test_photo.jpg
executing: convert -size 2550x1750 canvas:white public/temp/test_photo.jpg -geometry 1200x800+50+50 -composite public/temp/test_photo.jpg -geometry 1200x800+1300+50 -composite public/temp/test_photo.jpg -geometry 1200x800+50+900 -composite public/temp/test_photo.jpg -geometry 1200x800+1300+900 -composite public/temp/test_photo.jpg -geometry  -composite public/temp/test_photo.jpg -geometry  -composite public/temp/test_photo.jpg -geometry  -composite public/temp/test_photo.jpg -geometry  -composite public/temp/test_photo.jpg -geometry  -composite public/temp/test_photo.jpg -geometry  -composite public/temp/test_photo.jpg -geometry  -composite public/temp/test_photo.jpg -geometry  -composite public/temp/test_photo.jpg -geometry  -composite public/temp/test_photo.jpg -geometry  -composite public/temp/test_photo.jpg -geometry  -composite public/temp/test_photo.jpg -geometry  -composite public/temp/out.jpg

/Users/ben/Github/shmile-bsalinas/lib/image_compositor.coffee:64
              throw err;
                    ^
Error: Command failed: convert: invalid argument for option `-geometry': undefined @ error/convert.c/ConvertImageCommand/1674.

  at ChildProcess.<anonymous> (/Users/ben/Github/shmile-bsalinas/node_modules/imagemagick/imagemagick.js:88:15)
  at ChildProcess.emit (events.js:98:17)
  at maybeClose (child_process.js:756:16)
  at Socket.<anonymous> (child_process.js:969:11)
  at Socket.emit (events.js:95:17)
  at Pipe.close (net.js:465:12)

This resulted, in at least one case, from the browser refreshing. This caused a new websocket connection to be created, which caused a new camera to be created and the listener placed on that camera to trigger one for each connection that once existed.

I moved the camera constructor out of the websocket event, thinking that there should probably only be one camera object anyway. I also brought the code that adds images to the State.image_src_list outside of the websocket event so it only gets bound once.

I haven't actually been able to test this with a real camera but it seems to solve the issue when using the simulated camera.

andrewhao commented 9 years ago

@bsalinas I think this looks good - I hacked this app together years ago with no thought to architecture so some high-level architecture review is in order for this app. The crux of the problem is more likely due to the usage of global state in the app - I think a better approach will be to remove reliance of global image state and keeping it local to each session (e.g. each web session). However, the problem then becomes that the 'photo_saved' callback needs to keep a pointer to which session has requested it - then we have the whole issue with how we manage sessions per browser refresh.

Anyways, there is much work to be done here. If this fix works for you - and I suspect it's completely harmless, then go ahead and merge it.

:+1:

andrewhao commented 9 years ago

There is one camera (shared resource) between potentially multiple web clients. Thus we might even think of decoupling the relationship between the two with a pub/sub queue or equivalent.

danrue commented 9 years ago

+1 Please merge.

Using this branch, I ran a photo booth last night for 4 hours and it did not crash once. Last time I ran one back in October 2014, it crashed frequently.

andrewhao commented 9 years ago

I'll think about how to make this testable. Until then, @danrue's feedback is enough to convince me of its stability.