Autodesk-Forge / learn.forge.designautomation

Learn Forge Tutorial: Modify your models using Design Automation (AutoCAD, Inventor, Revit & 3dsMax). Available in .NET and Nodejs.
http://learnforge.autodesk.io
MIT License
39 stars 40 forks source link

Fixed bug: data corruption on multiple open tabs/clients #17

Closed pranavtotla closed 4 years ago

pranavtotla commented 4 years ago

A small bug which would transmit downloadResult and onComplete to all open socket clients, as the to param in socketIO was missing. Now emits response to the right client only.

libvarun commented 4 years ago

in documentation it's mentioned as room, not as id, it's not clear whether it applies to single client. https://socket.io/docs/server-api/#socket-to-room

Also in https://socket.io/docs/emit-cheatsheet/ there's warning message regarding usage of io.to(socketId)

Please elaborate if i'm missing anything.

pranavtotla commented 4 years ago

Firstly, you can simulate the problem I'm saying using the following steps:

However, with the proposed change, only the tab that you start the activity gets the result.

If you check the current code, socketIO.to(req.query.id).emit() has already been used onComplete (DesignAutomation.js, line 616 and 628 in the current nodejs branch). This is the reason why you do not get the success / failure report on all open tabs.

As socketIO.emit() has been used for downloadResult, you see Download result on all the open tabs.

Also, https://socket.io/docs/emit-cheatsheet/ clearly gives the use case of io.to, that is:

// sending to individual socketid (private message)
io.to(socketId).emit('hey', 'I just met you');

The warning is for using a socket object, but in DesignAutomation.js, the socketIO is actually an io object. Using io.to().emit() is cool, probably.

libvarun commented 4 years ago

Thanks, i was able to reproduce. Will be merged soon.