Repository navigation
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #851 +/- ##
==========================================
+ Coverage 91.54% 91.68% +0.13%
==========================================
Files 235 235
Lines 28787 28980 +193
==========================================
+ Hits 26353 26570 +217
+ Misses 2434 2410 -24 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
PengZheng
left a comment
PengZheng
left a comment
There was a problem hiding this comment.
Nice improvement for our networking code.
The HTTP code is good enough except for its zero return handling.
The TCP part is more difficult to deal with raw socket API.
PengZheng
left a comment
PengZheng
left a comment
There was a problem hiding this comment.
I left some further remarks, some of which need to be addressed before merging this PR.
| for (i = 0; i < length; i += 1) { | ||
| connection_pt connection = celix_arrayList_get(instance->connections, i); | ||
| connection->threadRunning = false; | ||
| connection_setThreadRunning(connection, false); |
There was a problem hiding this comment.
I have found no synchronization between connection processing thread and the calling thread, and thus suspect use-after-free bugs here.
Moreover, we never calling pthread_join on these connection processing thread, these are definitely resource leaks.
There was a problem hiding this comment.
remoteShell_stopConnections is only invoked when bundle stops, thus the resources used by these threads will only be released then. If new connection keeps coming, the overall resource usage will increase monotonically. Kind of resource leak. @pnoltes
PengZheng
left a comment
PengZheng
left a comment
There was a problem hiding this comment.
I have left some further remarks.
| } | ||
| totalBytesRead += (size_t)read; | ||
| } | ||
| if (contentLength > 0 && totalBytesRead < (size_t)contentLength) { |
There was a problem hiding this comment.
If fclose(celix_streal_ptr(stream)) before this, could we replace memSize by totalBytesRead so that we can also drop totalBytesRead += (size_t)read? Of cource we need to fflush the stream after fwrite to get totalBytesRead updated.
| return 400; | ||
| } | ||
| } | ||
| *bufferOut = celix_steal_ptr(buffer); |
There was a problem hiding this comment.
Note that buffer could be changed (and thus made invalid AND obsolete) by the final implicit fclose.
| for (i = 0; i < length; i += 1) { | ||
| connection_pt connection = celix_arrayList_get(instance->connections, i); | ||
| connection->threadRunning = false; | ||
| connection_setThreadRunning(connection, false); |
There was a problem hiding this comment.
remoteShell_stopConnections is only invoked when bundle stops, thus the resources used by these threads will only be released then. If new connection keeps coming, the overall resource usage will increase monotonically. Kind of resource leak. @pnoltes
Improve remote shell loop by
recvreturn and then logging result and exiting loop if needed.