Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[apps] Fix broken TcpMedium of srt-tunnel in windows #935

Merged
merged 1 commit into from
Nov 20, 2019
Merged

[apps] Fix broken TcpMedium of srt-tunnel in windows #935

merged 1 commit into from
Nov 20, 2019

Conversation

sorayuki
Copy link
Contributor

@sorayuki sorayuki commented Nov 2, 2019

close(fd), write(fd, ...) and read(fd, ...) are not suitable for socket operation in Windows.
I have changed them to closesocket, send and read, while use read/write for other platforms to prevent sigpipe signal.

why use shutdown for tcp_close:
that is what udpcommon class in transmitmedia.cpp does.

@sorayuki
Copy link
Contributor Author

sorayuki commented Nov 2, 2019

I find the tunnel very easy to freeze in Windows.

It is freezed in a waiting-chain like this.
wait-chain
The main thread, which is waiting for a slow recv of another socket indirectly, stops accepting new incoming connections.

@ethouris
Copy link
Collaborator

ethouris commented Nov 4, 2019

Deadlocks like that typically happen in case when a thread is expected to exit by itself, and that's why it's being joined, but by some reason it doesn't exit.

Would you be able to run it with -v option and make it freeze? How easy is it to make it?

@maxsharabayko maxsharabayko added [apps] Area: Test applications related improvements Type: Bug Indicates an unexpected problem or unintended behavior labels Nov 4, 2019
@maxsharabayko maxsharabayko added this to the v1.4.2 milestone Nov 4, 2019
@sorayuki
Copy link
Contributor Author

sorayuki commented Nov 8, 2019

The structure is like this:

image

The browser, the 2 tunnels and the proxy are running in the same machine.

And these are log files:
log.zip

It's very easy to make it by visiting some foreign website in browser.
It doesn't happen if browser connect to the proxy without the 2 tunnels.

I'm currently not very sure if the problem caused by my win32-porting or not.

@maxsharabayko maxsharabayko modified the milestones: v1.4.2, v1.4.1 Nov 8, 2019
apps/srt-tunnel.cpp Outdated Show resolved Hide resolved
apps/srt-tunnel.cpp Outdated Show resolved Hide resolved
@sorayuki
Copy link
Contributor Author

code is edited, and rebased to lastest master branch.
Add setsockopts in case of SIGPIPE

@@ -478,6 +490,12 @@ class TcpMedium: public Medium
// Just models. No options are predicted for now.
void ConfigurePre(int )
{
int optval = 1;
#if defined(LINUX)
setsockopt(m_socket, SOL_SOCKET, MSG_NOSIGNAL, (const char*)&optval, sizeof(optval));
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

MSG_NOSIGNAL is not a flag settable by setsockopt. It's a flag that should be passed in send function in the last argument.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

https://stackoverflow.com/questions/26752649/so-nosigpipe-was-not-decleared

It is also recommended that if this is simply a standalone application (not a library), you can simply disable the SIGPIPE signal.

Copy link
Collaborator

@ethouris ethouris left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please fix the MSG_NOSIGNAL call

@sorayuki
Copy link
Contributor Author

sorayuki commented Nov 11, 2019

Done.
I have got no Linux or MacOS environment at the minute. I'd like to rely on your CI system to check if it builds.

According to How to prevent SIGPIPEs (or handle them properly), neither MSG_NOSIGNAL nor SO_NOSIGPIPE is portable.

Another problem, I found cmake fail to generator nmakefile since commit 291bdf2d97bc23444a24fdd505ffdee7134e70e9, unless I change the last line of srtcore/filelist.maf from ../version.h to version.h.

CMake commandline is

cmake -DCMAKE_MSVC_RUNTIME_LIBRARY=MultiThreaded
	  -DCMAKE_POLICY_DEFAULT_CMP0091=NEW 
	  -DCMAKE_PREFIX_PATH="C:/Users/SoraYuki/source/repos/srt_build/dist" 
	  -G "NMake Makefiles" 
	  -DCMAKE_BUILD_TYPE=Release
	  -DCMAKE_INSTALL_PREFIX="C:/Users/SoraYuki/source/repos/srt_build/dist"
	  -S "C:/Users/SoraYuki/source/repos/srt_build/srt"
	  -B "C:/Users/SoraYuki/source/repos/srt_build/srt/build"
	  -DUSE_ENCLIB=mbedtls
	  -DENABLE_SHARED=ON 
	  -DENABLE_APPS=0
	  -DENABLE_STATIC=OFF

It reports

-- Configuring done
CMake Error at CMakeLists.txt:615 (add_library):
  Cannot find source file:

	C:/Users/SoraYuki/source/repos/srt_build/srt/version.h

  Tried extensions .c .C .c++ .cc .cpp .cxx .cu .m .M .mm .h .hh .h++ .hm
  .hpp .hxx .in .txx


CMake Error at CMakeLists.txt:615 (add_library):
  No SOURCES given to target: srt_shared

In fact the file is located in C:/Users/SoraYuki/source/repos/srt_build/srt/build

After the changes, I have tried not to use -S and -B, just use . as what CI script does. It also works.

@ethouris
Copy link
Collaborator

Placing version.h file in the filelist.maf file is completely wrong. I don't know how it slipped through my hands, but this couldn't work. The "maf" system is intended to provide STATIC list of files located at the exactly pointed location, relative to the directory where the filelist.maf file is located. The build directory location is to be decided dynamically. Adding this file makes the build broken, ALWAYS, either direct or shadow build, depending on which path you use.

Not sure how to approach this change, but if you figure out how to make the build system work without this version.h in filelist.maf, I will be grateful. If this file REALLY needs to be added to the file list, it better be added directly in CMakeLists.txt near the place where the configure_file call occurs.

@maxsharabayko
Copy link
Collaborator

Placing version.h file in the filelist.maf file is completely wrong. I don't know how it slipped through my hands, but this couldn't work

Slipped in PR #639

@maxsharabayko
Copy link
Collaborator

filelist.maf fix in #960

@maxsharabayko
Copy link
Collaborator

@sorayuki Please update the PR against master branch (conflicting changes).

@sorayuki
Copy link
Contributor Author

done

apps/srt-tunnel.cpp Outdated Show resolved Hide resolved
@rndi rndi merged commit ffb8039 into Haivision:master Nov 20, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[apps] Area: Test applications related improvements Priority: High Type: Bug Indicates an unexpected problem or unintended behavior
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants