sourcey / libsourcey

C++14 evented IO libraries for high performance networking and media based applications
https://sourcey.com/libsourcey
GNU Lesser General Public License v2.1
1.32k stars 350 forks source link

FormWriter cannot send multiple parts #171

Closed Malesio closed 6 years ago

Malesio commented 7 years ago

Hi Kam!

Back to finding things to improve in LibSourcey, I've been quite interested in the FormWriter class behaviour. After several tests, it appears that the class isn't able to send multiple form parts.

I mean, the class relies on ProgressSignal to dispatch the reading progress of the parts. However, its fields are never initialised properly. So, one part just works because of this :

void update(int nread)
{
    assert(current <= total);
    current += nread;
    emit(progress());
}

The assertion is what's making the test to fail. Before adding the first part, current and total are both equal to 0, so the assertion succeeds. After this call, current would hold the size of the part that was just added, while total is still 0, since it is never updated anywhere (grep -r 'OutgoingProgress.total' . in src/http/ told me so), and the assertion fails on adding another part.

Although it causes a crash in debug mode, running this in release mode will indeed lead into bogus progress values, such as things greater than 1 (>100%). Also, this :

double progress() const { return (current / (total * 1.0)) * 100; }

will lead to a division by zero, returning NaN (or inf).

I am not quite sure how this could be fixed, so I just posted it here as an issue. I have an idea though, as total should be computed before writeMultipartChunk gets called.

That's kind of a minor bug, but I think that it is worth reporting, though.

auscaster commented 7 years ago

Thanks very much for this. If you fixed it locally please feel free to submit a PR and I will merge it, or I will make the fix soon! :)

Malesio commented 6 years ago

Hi there, I'm back somehow!

I recently managed to figure out the problem about this whole failed assertion thingy, it was simply the total member related to the OutgoingProgress field of the ConnectionStream instance in the FormWriter that was not being updated correctly.

Actually, I found that one upon looking here. I was wondering why the line that is supposed to set the total field is commented out. Is there any reason for this ? (I've tested uncommenting it, and that worked pretty well so far. When registering a slot to the OutgoingProgress and logging the progress, it effectively went up to 100%).

auscaster commented 6 years ago

Not sure why that was commented out, it's now reenabled.